In this case I had already added the braces around the if clause before Jeff had sent that email, so Coverity is happy with it now.
-Ian On Sun, Jan 12, 2020 at 12:38 PM Wayne Stambaugh <stambau...@gmail.com> wrote: > In the case of a false positive, we should just set the "Classification" > to "False Positive" and the "Action" to "Ignore" so that it wont keep > showing up in the report. We should also add a comment as to why it's a > false positive so we know why it was marked as a false positive. > > Cheers, > > Wayne > > On 1/12/20 7:31 AM, Jeff Young wrote: > > I agree with Ian — looks like a false positive on closer inspection. > > > > The cost of requiring braces on single-line statements is one of a > > reduction in signal-to-noise ratio (and I suppose less fitting on the > > screen). > > > > Cheers, > > Jeff. > > > > > >> On 12 Jan 2020, at 01:32, Bevan Weiss <bevan.we...@gmail.com > >> <mailto:bevan.we...@gmail.com>> wrote: > >> > >> Is there any harm in having braces for each if / else / for / while > >> statement (regardless of if it’s single line or not)? > >> There is the possibility of harm in not doing it, so it seems that > >> adding it to the code style rules, would be prudent. > >> > >> That Coverity is throwing a positive at this implies that it is not > >> ‘good’ coding practise in this instance (given it’s a macro being > >> called, and the macro could easily expand to something which causes > >> issues in the Kicad codebase, but nowhere else). > >> > >> - Bevan > >> > >> *From:* Kicad-developers > >> <kicad-developers-bounces+bevan.weiss=gmail....@lists.launchpad.net > >> <mailto:kicad-developers-bounces+bevan.weiss= > gmail....@lists.launchpad.net>> *On > >> Behalf Of *Ian McInerney > >> *Sent:* 12 January 2020 12:24 > >> *To:* Jeff Young <j...@rokeby.ie <mailto:j...@rokeby.ie>> > >> *Cc:* KiCad Developers <kicad-developers@lists.launchpad.net > >> <mailto:kicad-developers@lists.launchpad.net>> > >> *Subject:* Re: [Kicad-developers] Coverity finds an ugly bug in > wxWidgets > >> > >> I thought that Wayne was agreeing with requiring them on all lengths > >> of statements (single line). > >> > >> Although, now that I look at the code closer, I don't know if this is > >> as dire a problem. While it is a macro, it is defined like this: > >> #define wxLogTrace > >> \ > >> if ( !wxLog::IsLevelEnabled(wxLOG_Trace, wxLOG_COMPONENT) ) > >> \ > >> {} > >> \ > >> else > >> \ > >> wxMAKE_LOGGER(Trace).LogTrace > >> > >> so it has an else statement already included in it. That means that > >> any else condition following it will be placed with the next higher > >> level of if statements outside the macro (and there is no worry about > >> having an else-if associated with this, since the else must come after > >> all else-if statements, meaning that this if condition is effectively > >> ended). > >> > >> The wxASSERT macro is an example of a case where they did introduce a > >> guard scope around the macro contents (by placing it inside a do-while > >> loop that only executes the first pass), since it is an if with only > >> an else-if statement and no else statement. If they hadn't given it > >> that scoping, then any else statements in user code that follows it > >> would be associated with the wxASSERT if statement instead. > >> > >> It seems to me that on closer inspection, this may just be Coverity > >> throwing a false positive at us. > >> > >> -Ian > >> > >> On Sun, Jan 12, 2020 at 12:42 AM Jeff Young <j...@rokeby.ie > >> <mailto:j...@rokeby.ie>> wrote: > >>> We could also just require them on if/then/else statements…. > >>> > >>> > >>>> On 12 Jan 2020, at 00:35, Jeff Young <j...@rokeby.ie > >>>> <mailto:j...@rokeby.ie>> wrote: > >>>> > >>>> Sure, but unless we go with Seth’s option, then it’s just going to > >>>> happen again…. > >>>> > >>>> > >>>>> On 11 Jan 2020, at 23:28, Wayne Stambaugh <stambau...@gmail.com > >>>>> <mailto:stambau...@gmail.com>> wrote: > >>>>> > >>>>> I agree that adding the curly brackets would be the best option as > >>>>> well. > >>>>> It's less than ideal but it resolves the issue. > >>>>> > >>>>> On 1/11/20 6:21 PM, Ian McInerney wrote: > >>>>> > >>>>>> That is probably the best option, since many things in wxWidgets are > >>>>>> implemented as macros but masquerade as functions. > >>>>>> > >>>>>> -Ian > >>>>>> > >>>>>> On Sat, Jan 11, 2020 at 10:07 PM <s...@kipro-pcb.com > >>>>>> <mailto:s...@kipro-pcb.com> > >>>>>> <mailto:s...@kipro-pcb.com>> wrote: > >>>>>> > >>>>>> I suppose that we could update our coding policy to require > braces > >>>>>> even for single line statements. > >>>>>> > >>>>>> -Seth > >>>>>> > >>>>>> On Jan 11, 2020 1:28 PM, Jeff Young <j...@rokeby.ie > >>>>>> <mailto:j...@rokeby.ie> > >>>>>> <mailto:j...@rokeby.ie>> wrote: > >>>>>> > >>>>>> This looks safe enough: > >>>>>> > >>>>>> if( n_changed ) > >>>>>> wxLogTrace( "CN", "Cluster %p : net : %d %s\n", > >>>>>> cluster.get(), > >>>>>> cluster->OriginNet(), (const char*) > >>>>>> cluster->OriginNetName().c_str() ); > >>>>>> else > >>>>>> wxLogTrace( "CN", "Cluster %p : nothing to propagate\n", > >>>>>> cluster.get() ); > >>>>>> > >>>>>> > >>>>>> Sadly, the macro wxLogTrace is not parenthesized, and starts > >>>>>> with an if statement. So the else doesn’t go where you > >>>>>> think it > >>>>>> does…. > >>>>>> > >>>>>> Any ideas on how to fix this that don’t include constantly > >>>>>> checking to see if new instances have been introduced? > >>>>>> > >>>>>> > >>>>>> _______________________________________________ > >>>>>> Mailing list: https://launchpad.net/~kicad-developers > >>>>>> Post to : kicad-developers@lists.launchpad.net > >>>>>> <mailto:kicad-developers@lists.launchpad.net> > >>>>>> <mailto:kicad-developers@lists.launchpad.net> > >>>>>> Unsubscribe : https://launchpad.net/~kicad-developers > >>>>>> More help : https://help.launchpad.net/ListHelp > >>>>>> > >>>>>> > >>>>>> _______________________________________________ > >>>>>> Mailing list: https://launchpad.net/~kicad-developers > >>>>>> Post to : kicad-developers@lists.launchpad.net > >>>>>> <mailto:kicad-developers@lists.launchpad.net> > >>>>>> Unsubscribe : https://launchpad.net/~kicad-developers > >>>>>> More help : https://help.launchpad.net/ListHelp > >>>>>> > >>>>> > >>>>> _______________________________________________ > >>>>> Mailing list: https://launchpad.net/~kicad-developers > >>>>> Post to : kicad-developers@lists.launchpad.net > >>>>> <mailto:kicad-developers@lists.launchpad.net> > >>>>> Unsubscribe : https://launchpad.net/~kicad-developers > >>>>> More help : https://help.launchpad.net/ListHelp > >>>> > >>>> _______________________________________________ > >>>> Mailing list: https://launchpad.net/~kicad-developers > >>>> Post to : kicad-developers@lists.launchpad.net > >>>> <mailto:kicad-developers@lists.launchpad.net> > >>>> Unsubscribe : https://launchpad.net/~kicad-developers > >>>> More help : https://help.launchpad.net/ListHelp > >>> > >>> _______________________________________________ > >>> Mailing list: https://launchpad.net/~kicad-developers > >>> Post to : kicad-developers@lists.launchpad.net > >>> <mailto:kicad-developers@lists.launchpad.net> > >>> Unsubscribe : https://launchpad.net/~kicad-developers > >>> More help : https://help.launchpad.net/ListHelp > >> _______________________________________________ > >> Mailing list: https://launchpad.net/~kicad-developers > >> Post to : kicad-developers@lists.launchpad.net > >> <mailto:kicad-developers@lists.launchpad.net> > >> Unsubscribe : https://launchpad.net/~kicad-developers > >> More help : https://help.launchpad.net/ListHelp > > > > > > _______________________________________________ > > Mailing list: https://launchpad.net/~kicad-developers > > Post to : kicad-developers@lists.launchpad.net > > Unsubscribe : https://launchpad.net/~kicad-developers > > More help : https://help.launchpad.net/ListHelp > > > > _______________________________________________ > Mailing list: https://launchpad.net/~kicad-developers > Post to : kicad-developers@lists.launchpad.net > Unsubscribe : https://launchpad.net/~kicad-developers > More help : https://help.launchpad.net/ListHelp >
_______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
