Opened 9 years ago

Closed 6 years ago

Last modified 6 years ago

#638 closed defect (fixed)

Recommend a static_cast around assert strings

Reported by: goatbar Owned by: geos-devel@…
Priority: major Milestone: 3.7.0
Component: Default Version: 3.3.8
Severity: Unassigned Keywords:
Cc:

Description (last modified by robe)

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"));

Change History (19)

comment:1 by goatbar, 9 years ago

warmerdam suggested:

Perhaps assertFailureWithMessage(text) that expands to something like this

comment:2 by mloskot, 9 years ago

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.

comment:3 by goatbar, 9 years ago

"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'

comment:4 by robe, 9 years ago

Description: modified (diff)
Milestone: 3.5.0

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

comment:5 by robe, 7 years ago

Milestone: 3.5.03.6.0

comment:6 by strk, 6 years ago

Milestone: 3.6.03.7.0

Ticket retargeted after milestone closed

comment:7 by goatbar, 6 years ago

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

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

    diff --git a/geos/geos-3.6.1/src/geom/util/GeometryEditor.cpp b/geos/geos-3.6.1/src/geom/util/GeometryEditor.cpp
    index c70fa1e..0f9b589 100644
    a b GeometryEditor::edit(const Geometry *geometry, GeometryEditorOperation *operatio  
    9999  }
    100100
    101101    // Unsupported Geometry classes should be caught in the GeometryEditorOperation.
    102     assert(!"SHOULD NEVER GET HERE");
     102    assert(!static_cast<bool>("SHOULD NEVER GET HERE"));
    103103    return NULL;
    104104}
    105105
  • geos/geos-3.6.1/src/index/strtree/AbstractSTRtree.cpp

    diff --git a/geos/geos-3.6.1/src/index/strtree/AbstractSTRtree.cpp b/geos/geos-3.6.1/src/index/strtree/AbstractSTRtree.cpp
    index f11401a..84e7a86 100644
    a b ItemsList* AbstractSTRtree::itemsTree(AbstractNode* node)  
    370370                static_cast<ItemBoundable*>(childBoundable)->getItem());
    371371        }
    372372        else {
    373             assert(!"should never be reached");
     373            assert(!static_cast<bool>("should never be reached"));
    374374        }
    375375    }
    376376    if (valuesTreeForNode->empty())
    ItemsList* AbstractSTRtree::itemsTree()  
    395395} // namespace geos.index.strtree
    396396} // namespace geos.index
    397397} // namespace geos
    398 
  • geos/geos-3.6.1/src/linearref/ExtractLineByLocation.cpp

    diff --git a/geos/geos-3.6.1/src/linearref/ExtractLineByLocation.cpp b/geos/geos-3.6.1/src/linearref/ExtractLineByLocation.cpp
    index 04b3690..4928da5 100644
    a b Geometry *ExtractLineByLocation::reverse(const Geometry *linear)  
    7979                }
    8080                else
    8181                {
    82                         assert(!"non-linear geometry encountered");
     82                        assert(!!static_cast<bool>("non-linear geometry encountered"));
    8383            return 0;
    8484                }
    8585        }
  • geos/geos-3.6.1/src/operation/union/CascadedPolygonUnion.cpp

    diff --git a/geos/geos-3.6.1/src/operation/union/CascadedPolygonUnion.cpp b/geos/geos-3.6.1/src/operation/union/CascadedPolygonUnion.cpp
    index 92ec6da..64e66c7 100644
    a b CascadedPolygonUnion::reduceToGeometries(index::strtree::ItemsList* geomTree)  
    139139            geoms->push_back(reinterpret_cast<geom::Geometry*>((*i).get_geometry()));
    140140        }
    141141        else {
    142             assert(!"should never be reached");
     142            assert(!static_cast<bool>("should never be reached"));
    143143        }
    144144    }
    145145
    CascadedPolygonUnion::restrictToPolygons(std::auto_ptr<geom::Geometry> g)  
    247247} // namespace geos.operation.union
    248248} // namespace geos.operation
    249249} // namespace geos
    250 
  • geos/geos-3.6.1/src/operation/union/CascadedUnion.cpp

    diff --git a/geos/geos-3.6.1/src/operation/union/CascadedUnion.cpp b/geos/geos-3.6.1/src/operation/union/CascadedUnion.cpp
    index 4bd522a..744dae6 100644
    a b CascadedUnion::reduceToGeometries(index::strtree::ItemsList* geomTree)  
    116116            geoms->push_back(reinterpret_cast<geom::Geometry*>((*i).get_geometry()));
    117117        }
    118118        else {
    119             assert(!"should never be reached");
     119            assert(!static_cast<bool>("should never be reached"));
    120120        }
    121121    }
    122122
    CascadedUnion::unionActual(geom::Geometry* g0, geom::Geometry* g1)  
    195195} // namespace geos.operation.union
    196196} // namespace geos.operation
    197197} // namespace geos
    198 

comment:8 by strk, 6 years ago

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

comment:9 by goatbar, 6 years ago

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

comment:10 by strk, 6 years ago

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)

comment:11 by mloskot, 6 years ago

To me, the cast looks, well, unfitting.

Why not to use conversion to bool, idiomatic

assert(!!!"SHOULD NEVER GET HERE");
Last edited 6 years ago by mloskot (previous) (diff)

comment:12 by goatbar, 6 years ago

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

comment:13 by goatbar, 6 years ago

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

comment:15 by strk, 6 years ago

Resolution: fixed
Status: newclosed

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

comment:16 by strk, 6 years ago

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

comment:17 by Sandro Santilli <strk@…>, 6 years ago

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

comment:18 by Sandro Santilli <strk@…>, 6 years ago

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

comment:19 by Sandro Santilli <strk@…>, 6 years ago

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

Note: See TracTickets for help on using tickets.