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
