On Aug 24, 2015 6:29 AM, "David Bennett" <davidb at pfxcorp.com> wrote: > > I think we've beaten the philosophy to death and we're largely in agreement. > > I'm not sure we actually came up with a firm recommendation as to what to do > about this specific warning in this particular line of code with this > compiler. Ostrich treatment maybe, and rely on the general Sqlite > disclaimer?
Well, I haven't inspected the code closely. My discussions have been purely philosophical. :) If the warning has to do with a constant zero expression, then I think I'd probably go ahead and add the if statement at this point, giving the optimizer the opportunity to completely remove the code fragment, even though that would likely just change warnings. If the expression is not constant, I don't have a strong opinion on how to suppress or if to suppress. Given SQLite's focus on performance (and that the code is correct either way) I'd probably profile and see if performance was impacted and make the decision then. If performance was clearly better with an if statement I'd make the change. If not suppress the warning conditionally if possible (gcc is a very common choice of compiler and worth having a clean build) or ignore it if not. > > Regards > David M Bennett FACS > > MD Powerflex Corporation, creators of PFXplus > To contact us, please call +61-3-9548-9114 or go to > www.pfxcorp.com/contact.htm > > -----Original Message----- > From: sqlite-users-bounces at mailinglists.sqlite.org > [mailto:sqlite-users-bounces at mailinglists.sqlite.org] On Behalf Of Scott > Robison > Sent: Monday, 24 August 2015 8:25 AM > To: davidb at pfxcorp.com; General Discussion of SQLite Database > <sqlite-users at mailinglists.sqlite.org> > Subject: Re: [sqlite] Compile warnings > > On Sat, Aug 22, 2015 at 8:07 PM, David Bennett <davidb at pfxcorp.com> wrote: > > > Of course that is the aim, as always. > > > > > > > > In this particular case, maximally portable code (that will compile > > and execute correctly on all conforming compilers) must (a) ensure > > that the pointer argument is valid (b) ensure that the length is valid, or > zero. > > Where reasonably possible both should be done statically. If > > conditional code is introduced then it must be tested with all branches > covered. > > > > Agreed, and in C89 NULL was a valid pointer argument in this context as long > as the length was zero. That has nothing to do with this particular thread, > but I referenced it just as a point of "C99 changed the semantics of what is > valid, invalidating previously valid code". Projects that took advantage of > that can either modify their code to accommodate the newer standard or leave > it along claiming it conforms to the intended / desired standard. In that > case, it was easy to change to code to be compatible with both standards and > it was done. In this case (gcc warning about possible argument order error > due to a constant expression) can be equally accommodated, but it's not a > matter of standards compliance. It *is* a matter of one of the (or perhaps > *the*) most used compilers bellyaching about something that is not wrong or > invalid in any way. So is the code modified to suppress the warning, is the > warning disabled globally, locally, or just ignored? I can see the benefits > to making the change and to not making the change. > > > As always, it may be the case that one or more compilers may issue > > warnings for code that is fully compliant with the standard and fully > > tested. Sadly there may even be compilers that compile the code > > incorrectly (a compiler bug). The question of how to handle undesired > > warnings or compiler bugs on code that is known to be correct and > > compliant is always a judgement call. In my opinion the solution > > chosen should always be as fine-grained as possible (such as a > > compiler-specific conditional), but the downside is that the code can > > become littered with references to specific problems in specific compilers > and thus become harder to work with. > > > > I agree completely. Most of us don't have to worry about this too much > because we target one platform (most code is not written with portability in > mind). SQLite is not most projects. > > > > In my opinion changes that are visible to other compilers should be > > avoided unless the changed code is an equally valid solution to the > > problem at hand and/or the problem affects multiple compilers. In this > > light adding an if-test would be an incorrect choice (and would > > require an additional set of tests). Suppressing the warning > > specifically for this compiler would be a preferable solution. > > > > Agreed for the most part. Just to be clear: I never said "don't make this > change" (I don't think). I just wanted to bring up the thought that an if > statement *might* degrade performance in a code base that has been carefully > tuned to maximize efficiency. I wasn't thinking of test cases or such, just > efficiency. That being said, if gcc is generating the warning because a > constant expression has a value of zero, an if statement might actually > increase performance by providing the compiler with the knowledge that it > can completely remove the corresponding memset because the expression will > always be false. In that case, add the if might be the exact right thing to > do. > > Again we're to the point of "there is no one universal right solution to > this issue". The only thing I can say is: not all warnings are equally bad, > and I will review warnings that are generated from third party code (such as > SQLite) but I rarely will do anything to try to suppress them. Making my > code right is a hard enough task. I don't need to "fix" third party code (as > long as it passes testing). > > -- > Scott Robison > _______________________________________________ > sqlite-users mailing list > sqlite-users at mailinglists.sqlite.org > http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users > > > _______________________________________________ > sqlite-users mailing list > sqlite-users at mailinglists.sqlite.org > http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
