Hi Robert, On 20-03-2012, at 12:19, Robert Osfield wrote:
> HI Erik, > > > On 19 March 2012 22:20, Erik den Dekker <[email protected]> wrote: >> using clang I received several warnings like: >> >> [ 8%] Building CXX object src/osg/CMakeFiles/osg.dir/State.cpp.o >> /Users/erik/Projects/OpenSceneGraph/OpenSceneGraph-SVN/src/osg/State.cpp:1166:57: >> warning: use of logical '||' with constant operand >> [-Wconstant-logical-operand] >> _isVertexBufferObjectSupported = OSG_GLES2_FEATURES || OSG_GL3_FEATURES || >> osg::isGLExtensionSupported(_contextID,"GL_ARB_vertex_buffer_object"); >> ^ ~~~~~~~~~~~~~~~~ >> /Users/erik/Projects/OpenSceneGraph/OpenSceneGraph-SVN/src/osg/State.cpp:1166:57: >> note: use '|' for a bitwise operation >> _isVertexBufferObjectSupported = OSG_GLES2_FEATURES || OSG_GL3_FEATURES || >> osg::isGLExtensionSupported(_contextID,"GL_ARB_vertex_buffer_object"); >> ^~ >> | >> 1 warning generated. > > The clang warning you've "fixed" is pretty dumb and the changes are > extensive and for me are less readable. Coding wise there are > completely pointless, the original code is in no way ambiguous, so I'm > rejecting the submission. If you want to fix the warning then the > most appropriate thing to do would be to disable the stupid warning. I would like you to reconsider the submission because: 1) I don't see why the warning is dumb or stupid; it does expose genuine issues in code. It actually exposed the bug in the DXF plugin below! Recommending to disable it seems strange to me. 2) It would improve consistency in OSG. Generally, #defines of style OSG_xxx are used for: (a) conditional compilation (including header guards) or (b) as a poor men's function declaration (which is in many cases also not recommendable practice) But the OSG_GLxxx_FEATURES #defines are used for computation (The other exception being OSG_HEADER_HIGH and OSG_HEADER_LOW, which should really also be changed to consts) 3) These #defines is being misused to represent what is in fact a const. However, the #define is not type safe. The OSG_GLxxx_FEATURES are #defined to an int value (0 or 1), but are only being used in a bool context. 4) Using a #define when it is not necessary pollutes the global namespace of the users of OSG. 5) Note that I changed the casing / style of 'OSG_GLxxx_FEATURES' to 'osg::hasGLxxxFeatures' only for consistency. The exact name may be a point for discussion. 6) The original submission contains straightforward replacement of X by Y in a total of 36 lines of code (in a total of 18 files). Is this really too extensive? Then, you may want to consider this alternative submission which contains changes to 13 lines of code in 1 file (it does not actually change the names of OSG_GLxxx_FEATURES).
13046-Fix_Incorrect_#Define_Usage.tar.gz
Description: GNU Zip compressed data
7) I know this submission hardly changes the functionality of OSG. But I do not fully understand why you reject the chance to increase the quality (IMHO) of the OSG code. High quality code is certainly not only about functionality? >> There is still a similar warning that points to a fishy piece of code in the >> DXF plugin: >> >> [ 75%] Building CXX object >> src/osgPlugins/dxf/CMakeFiles/osgdb_dxf.dir/dxfEntity.cpp.o >> /Users/erik/Projects/OpenSceneGraph/OpenSceneGraph-SVN/src/osgPlugins/dxf/dxfEntity.cpp:410:22: >> warning: use of logical '&&' with constant operand >> [-Wconstant-logical-operand] >> (cv._int && 128 /*i.e. vertex is actually a face*/)) >> ^ ~~~ >> /Users/erik/Projects/OpenSceneGraph/OpenSceneGraph-SVN/src/osgPlugins/dxf/dxfEntity.cpp:410:22: >> note: use '&' for a bitwise operation >> (cv._int && 128 /*i.e. vertex is actually a face*/)) >> ^~ >> & >> /Users/erik/Projects/OpenSceneGraph/OpenSceneGraph-SVN/src/osgPlugins/dxf/dxfEntity.cpp:410:22: >> note: remove constant to silence this warning >> (cv._int && 128 /*i.e. vertex is actually a face*/)) >> ^~~~~~ >> 1 warning generated. >> >> >> But I am not sure what the correct code would be,.. (Use & instead of >> &&?),.. So a fix is not included here. > > This looks to be a genuine error and the appropriate fix is to use a > &. This fix is now checked into svn/trunk. > > Thanks, > Robert. > _______________________________________________ > osg-submissions mailing list > [email protected] > http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org Thank you for your considerations, Erik
_______________________________________________ osg-submissions mailing list [email protected] http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org
