Hi Paul,
On Wed, Jan 7, 2009 at 10:06 PM, Paul Martz <[email protected]> wrote: > Your recommendation below seems like it would depend on what type fxn() > returns, am I correct? > > For example, if fxn() returns an int (and 'a' is declared as an int), then > the following is correct: > "if ( (a = fxn()) != 0 ) > > But the above is incorrect - and possibly inefficient - if fxn() returns a > bool (and 'a' is declared as a bool), because that code would causes a bool > to be compared against an int (0). If fxn() returns a bool, the following > should be sufficient (and should not generate a warning IMHO): > "if ( a = fxn() )" > > This is off topic for a scene graph mailing list, but I would like to > prevent someone from going through the code and doing a blanket replace of > good code with bad code. I am very much of the same opinion, a blanket change to != 0 is just plain wrong. Even if you correct the type for a bool so you get: if ( (a=fxn())!=false) This is far more confusing to read, and therefore more prone to errors down the road. This also applies to the int version with != 0, you are basically adding superfluous code to fix a warning that is occurring or perfectly correct code. The problem is not in the code but the fact that warning can't differentiate between intended implementation and erroneous implementation. Adding extra syntax to get wrong a warning doesn't solve anything either, as you could add the extra syntax inappropriately and potentially mask a real error. The more extra syntax you have the more obfuscated the actual intention of the code is, the more likely a coder will miss an error. There is also the added burden on understanding both the intricacies of the C++ language and then who set of rules that the compilers add to work around their over zealous warnings that are produce for perfectly valid C++. In this particular case the C++ is designed to allow you to assign values to variables in a conditionally because it's a useful for writing more streamlined and readable code, but with these extra hoops you loose on readability and maintainability. This is absolutely not a step forward. I have to read lots of code in my role as maintainer of the OSG, I have read lots of other peoples code, the more different it is to read the more time it takes to do a review and the more likely I'm to make a mistake in the process. I don't take degradation in code maintainability lightly. We are all exposed to this issue too, some may differ in their preferences but well all have to deal with trying to understand what the code does, the cleaner the code is the easier it is to work with. In my this instance I feel that the suggested VS workaround for the inappropriate warning is not a step in the right direction and should not be adopted, others may feel differently, but in case of the OSG I won't be merging attempts to move across to the VS workaround. My current feeling is that we should try to suppress this and a few of the other misleading warnings via compile options rather that #pragma's in the headers. So if we have /W4 on then we should then disable the warnings that aren't helpful. We should also work towards removing the #pragma's from the headers completely, and work to ensure that the headers compile cleanly without warnings, so that 3rd party apps can go enabling/disabling warnings as their developers see fit. Robert. _______________________________________________ osg-users mailing list [email protected] http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org
