Hi David Could you send me the changes that specifically address the bugs. Bugs need to be fixed and clearly labelled as bugs, don't only do they address problems but they also serve something that others can learn from and avoid in future. Fixing bugs amongst other lower priority stuff and not clearly marking them is not helpful.
As for the Qt using the UNUSED macro and this somehow proving it's a good programming practice. Well Qt has lots of silly thing in it that frankly don't need us emulating. Adding code just to disable warnings is BAD Programming. I can't be less subtle about, if you are resorting to crap like this a big alarm should be going off in your head. If it's the compiling warning forcing your to do such hacks then it's time to quiten down the compile, if it's crappy code then fix the code. Whatever you don't do is go adding hacks to the code to workaround problems elsewhere in the design/implementation/compiler usage. So please post the bug fixes right away as single submissions. If there are warning fixes that aren't hacks then please send them in, but separately from bug fixes. If they are hacks in the code just to quiten down the code don't bother sending them, and don't bother hiding them amongst other genuine and good fixes as I'll reject the whole lot, I simply don't have the time to jumping through hoops fixing submissions. Cheers, Robert. On 1 July 2013 11:25, David Callu <[email protected]> wrote: > Hi Robert > > > I want to fix all warning for a long time. And When I start this > last week, I just think, "fix this one and this one only", and I found a bug > in > osg::Geometry, another one in osgViewer::WoWVxDisplay, > And I think "how many bug are hide by no relevant warning ?" so I take > all my courage and try to fix all warning. Finally, I spot another one bug > in dicom plugin in FileInfo copy constructor ReaderWriterDICOM.cpp:l1056 > > > > Have many warning that can be easily fixed like > const int myFunction { ... } : type qualifiers ignored on function > return type > or > int myFunction(int toto, int titi) : titi not used > Is a big error, There warning HIDE the real one like in osgsidebyside > example > warning: virtual base 'osg::Referenced' inaccessible in > 'SwitchDOFVisitor' due to ambiguity > or in ffmpeg plugin > warning: ‘int avcodec_decode_audio3(AVCodecContext*, int16_t*, int*, > AVPacket*)’ is deprecated (declared at > /usr/include/libavcodec/avcodec.h:4251) > warning: 'osgFFmpeg::FFmpegParameters::m_options' will be initialized > after > warning: 'AVInputFormat* osgFFmpeg::FFmpegParameters::m_format' > warning: comparison between signed and unsigned integer expressions > or in osgmovie example > warning: missing initializer for member 'SDL_AudioSpec::format > or in plugin 3ds > warning: comparison of unsigned expression >= 0 is always true > > > Warning are there to say us "Something is strange, is it a bug ???" > If yes, fix it and go on > if no, do something to fix the strange behaviours, and go on > but don't accumulate warning !!! > > > > I think no warning mark one step in the code quality. When an osg-use > compile it's code > and have many warning because osg have many warning, some of them think > What the hell, I steel have warning and I can't fix them, AArrrrggggg > > > The solution to disable warning is a facility that I accept to use with > care. > When the compilator say strange warning like VC++. > > > UNUSED macro is largely used in Qt code that compile on so many > platform/compilator > that we can say this technique is largely validated. > But yes, this is not really beautiful in the code, so just use it when there > are not other solution > to fix the warning is a good policy. perhaps add it at the beginning of the > function will > make more easy to do abstraction of this line of code. > > > So during my blitz on warnings :-), I fix 3 bug, one misusing of inheritance > and give more visibility for future warning. > I also learn deeply inheritance, take a look to ffmpeg code and try for the > first time clang compilator. > I do this Saturday night and I will not start a more complicated task like > finish the VAO submission or update serialization for osg::Array. If i have > not > finish the warning fix, I probably watch movie on the TV. So i don't loose > my time. > > > To conclude, I think code that produce warning is like my bad English. > This will not break understanding of the text, but this is better without > it. > > > Best Regards > David > > > > 2013/7/1 Robert Osfield <[email protected]> >> >> HI David, >> >> Over the weekend I've been thinking about OSG_UNUSED macro approach >> for quietening down irrelevant warnings and feel that it's just bad >> programming. I believe it's fundamentally wrong to be adding code to >> program for the sole purpose of quietening down warnings so am going >> to be reverting all the code assocatied with and will not be merging >> any other code. This practice is a bad practice and don't wish to see >> it promoted in the OSG core. >> >> Warnings are there to help spot bugs, having to add code to fix >> inappropriate warnings makes the code less readable, and with added >> line you add the possibility of introducing a bug, and what to say >> that todays hack around a warning doesn't get flagged as a warning in >> the next rev of a compiler. >> >> I'm miffed with having to jump through hoops to fix what I now >> consider is broken code, and regret not rejecting the submission in >> the first place. >> >> The right thing to do with warnings that are producing warnings that >> can't be fixed directly without hacks is to disable the warning, not >> by hacking the code but fixing the compiler settings. >> >> If you want to protest ask yourself this simple question, how many >> bugs have you fixed with this blitz on warnings? >> >> Next question is how many bugs could you have fixed in the same time >> you spent squashing these useful warnings? >> >> Robert. >> >> On 30 June 2013 02:48, David Callu <[email protected]> wrote: >> > Hi Robert, >> > >> > Here the second pass of warning fix >> > >> > all archive are in the form src/... or include/... or example/... >> > >> > >> > >> > example_refactoring.txz fix warning for osgsidebyside example. >> > This example define a class derived from osgGA::GUIEventHandler and >> > osg::NodeVisitor. >> > This is a very bad example of double inheritance. the warning say that >> > osg::Referenced base class >> > is inaccessible. I fix this by create 2 class one derived from >> > osgGA::GUIEventHandler, a second derived >> > from osg::NodeVisitor. >> > >> > >> > osgPlugin_OpenFlight fix some warning and replace ® by (R). >> > >> > >> > other_wrapper_warning.txz fix warning for REGISTER_OBJECT_WRAPPER macro. >> > I don't use OSG_UNUSED any more, but I redefined a macro >> > REGISTER_EMPTY_OBJECT_WRAPPER >> > >> > >> > wrapper_warning.txz fix warning in osgWrapper when function's parameter >> > is >> > not used. >> > >> > >> > to fix warning when variable is used in assert and not in code, >> > I define an OSG_ASSERT in osg/Export >> > >> > >> > I now build without warning with GCC and Clang compilator, excepted for >> > ffmpeg plugin >> > >> > >> > Cheers >> > David >> > >> > _______________________________________________ >> > osg-submissions mailing list >> > [email protected] >> > >> > http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org >> > >> _______________________________________________ >> osg-submissions mailing list >> [email protected] >> >> http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org > > > > _______________________________________________ > osg-submissions mailing list > [email protected] > http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org > _______________________________________________ osg-submissions mailing list [email protected] http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org
