Agreed. It's easy for me to say what I think should be done since I'm not writing or maintaining the code.
On Mon, Aug 24, 2015, 5:53 PM David Bennett <davidb at pfxcorp.com> wrote: > Fair enough. Bearing in mind that any new code path requires a new test > case and coverage. > > > > Regards > > David M Bennett FACS > > *Andl - A New Database Language - andl.org <http://andl.org>* > > > > > > *From:* Scott Robison [mailto:scott at casaderobison.com] > *Sent:* Tuesday, 25 August 2015 1:24 AM > *To:* General Discussion of SQLite Database < > sqlite-users at mailinglists.sqlite.org>; davidb at pfxcorp.com > > > *Subject:* Re: [sqlite] Compile warnings > > > > 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 >
