Recommend a static_cast around assert strings

[goatbar]

I recommend that for all asserts with strings, that they be changed to wrap the string in a static_cast. Not very exciting, but it makes the stricter compilers happier.

e.g. in src/geom/util/GeometryEditor.cpp

assert(!"SHOULD NEVER GET HERE");

changes to

assert(!static_cast<bool>("SHOULD NEVER GET HERE"));

[goatbar]

warmerdam suggested:

Perhaps assertFailureWithMessage(text) that expands to something like this

[mloskot]

I'm curious, what stricter compilers you mean? Non of these complain, using strict flags -pedantic -Wall -ansi -std=c++11: clang 3.4, gcc 4.8, Intell C/C++ 13.

[goatbar]

"GCC 4.7.x with numerous patches", but I'm not exactly sure. This is the message I'm getting:

geos-3.3.8/src/geom/util/GeometryEditor.cpp:103:13: error: implicit conversion turns string literal into bool: 'const char [22]' to 'bool' [-Werror,-Wstring-conversion]
include/assert.h:92:5: note: expanded from macro 'assert'

[robe]

Not seeing this particular error on my mingw gcc 4.8 built with cmake. Though I do see this somewhat troubling notice

In file included from c:/jenkins/geos/branches/3.4cmake/tests/xmltester/BufferResultMatcher.cpp:22:0:
C:/jenkins/geos/branches/3.4cmake/include/geos/geom/BinaryOp.h: In instantiation of 'std::auto_ptr<geos::geom::Geometry> geos::geom::BinaryOp(const geos::geom::Geometry*, const geos::geom::Geometry*, BinOp) [with BinOp = geos::operation::overlay::overlayOp]':
c:/jenkins/geos/branches/3.4cmake/tests/xmltester/BufferResultMatcher.cpp:80:58:   required from here
C:/jenkins/geos/branches/3.4cmake/include/geos/geom/BinaryOp.h:459:32: warning: overflow in implicit constant conversion [-Woverflow]
   long unsigned int maxScale = 1e16;

I'd just assume avoid these changes in a micro release especially since we plan to release soon and sounds like a lot of changes would need to be made to accommodate this. Pushing to 3.5

[strk]

Ticket retargeted after milestone closed

[goatbar]

Ping. I just had to remerge 3.6.1 downstream. Here is my patch:

geos/geos-3.6.1/src/geom/util/GeometryEditor.cpp

@@ -99 +99 @@
   }
 
     // Unsupported Geometry classes should be caught in the GeometryEditorOperation.
-    assert(!"SHOULD NEVER GET HERE");
+    assert(!static_cast<bool>("SHOULD NEVER GET HERE"));
     return NULL;
 }
 

geos/geos-3.6.1/src/index/strtree/AbstractSTRtree.cpp

@@ -370 +370 @@
                 static_cast<ItemBoundable*>(childBoundable)->getItem());
         }
         else {
-            assert(!"should never be reached");
+            assert(!static_cast<bool>("should never be reached"));
         }
     }
     if (valuesTreeForNode->empty()) 
@@ -395 +395 @@
 } // namespace geos.index.strtree
 } // namespace geos.index
 } // namespace geos
-

geos/geos-3.6.1/src/linearref/ExtractLineByLocation.cpp

@@ -79 +79 @@
                 }
                 else
                 {
-                        assert(!"non-linear geometry encountered");
+                        assert(!!static_cast<bool>("non-linear geometry encountered"));
             return 0;
                 }
         }

geos/geos-3.6.1/src/operation/union/CascadedPolygonUnion.cpp

@@ -139 +139 @@
             geoms->push_back(reinterpret_cast<geom::Geometry*>((*i).get_geometry()));
         }
         else {
-            assert(!"should never be reached");
+            assert(!static_cast<bool>("should never be reached"));
         }
     }
 
@@ -247 +247 @@
 } // namespace geos.operation.union
 } // namespace geos.operation
 } // namespace geos
-

geos/geos-3.6.1/src/operation/union/CascadedUnion.cpp

@@ -116 +116 @@
             geoms->push_back(reinterpret_cast<geom::Geometry*>((*i).get_geometry()));
         }
         else {
-            assert(!"should never be reached");
+            assert(!static_cast<bool>("should never be reached"));
         }
     }
 
@@ -195 +195 @@
 } // namespace geos.operation.union
 } // namespace geos.operation
 } // namespace geos
-

[strk]

How did you get trac to format your patches like that ? Do you have a publically-reachable git repository to fetch your changes from ? See git request-pull

[goatbar]

You can tell track to format a block of text. e.g. I did {{{#!patch

I can clone the repo and do a pull request. I haven't used request-pull before. Thanks for the pointer to ​https://git-scm.com/docs/git-request-pull

[strk]

You don't necessarely need the github machinery, just a publicly accessible branch in a git repo you can write to :)

That said, if you want to use a web UI mediated branch tracking issue feel free to use it on any of the available mirrors: github, gitlab or gogs (the latter lets you use the same account you're using to write here).

See wiki:CodeRepository (blindly trying the TracLink)

[mloskot]

To me, the cast looks, well, unfitting.

Why not to use conversion to bool, pretty idiomatic

assert(!!!"SHOULD NEVER GET HERE");

[goatbar]

mloskot: Can you please explain and give a reference why !!!"" is equivalent to !static_cast<bool>("")? I've never seen that in C++ for C strings.

If I try !!! in C++11/14, I get the same type of error:

src/operation/union/CascadedUnion.cpp:119:23: error: implicit conversion turns string literal into bool: 'const char [24]' to 'bool' [-Werror,-Wstring-conversion]
            assert(!!!"should never be reached");

I found this for *javascript* ​http://stackoverflow.com/questions/21154510/the-use-of-the-triple-exclamation-mark

​https://cboard.cprogramming.com/c-programming/133581-what-double-exclamation-mark-c-language.html#post994202

[goatbar]

Ah, I'm building in a world with -Wpointer-bool-conversion enabled.

​http://stackoverflow.com/questions/35797295/clang-how-to-disable-werror-wpointer-bool-conversion

[goatbar]

• ​https://github.com/libgeos/libgeos/pull/77
• ​https://travis-ci.org/libgeos/libgeos/builds/197328630 travis-ci passes

[strk]

In 4331:

Add static_assert<bool> to strings inside of assert calls

Allows compiling with -Wpointer-bool-conversion.

Patch by Kurt Schwehr <schwehr@…>
Closes #638

[strk]

In 4332:

Add static_assert<bool> to strings inside of assert calls

Allows compiling with -Wpointer-bool-conversion.

Patch by Kurt Schwehr <schwehr@…>
Closes #638

[Sandro Santilli <strk@…>]

In 9c7b893/git:

Add static_assert<bool> to strings inside of assert calls

Allows compiling with -Wpointer-bool-conversion.

Patch by Kurt Schwehr <schwehr@…>
Closes #638

git-svn-id: ​http://svn.osgeo.org/geos/trunk@4331 5242fede-7e19-0410-aef8-94bd7d2200fb

[Sandro Santilli <strk@…>]

In 9c7b893/git:

Add static_assert<bool> to strings inside of assert calls

Allows compiling with -Wpointer-bool-conversion.

Patch by Kurt Schwehr <schwehr@…>
Closes #638

git-svn-id: ​http://svn.osgeo.org/geos/trunk@4331 5242fede-7e19-0410-aef8-94bd7d2200fb

[Sandro Santilli <strk@…>]

In 9c7b893/git:

Add static_assert<bool> to strings inside of assert calls

Allows compiling with -Wpointer-bool-conversion.

Patch by Kurt Schwehr <schwehr@…>
Closes #638

git-svn-id: ​http://svn.osgeo.org/geos/trunk@4331 5242fede-7e19-0410-aef8-94bd7d2200fb