kicad-developers team mailing list archive

[Ian McInerney <Ian.S.McInerney@xxxxxxxx>]

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 <stambaughw@xxxxxxxxx>
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.weiss@xxxxxxxxx
> >> <mailto:bevan.weiss@xxxxxxxxx>> 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.com@xxxxxxxxxxxxxxxxxxx
> >> <mailto:kicad-developers-bounces+bevan.weiss=
> gmail.com@xxxxxxxxxxxxxxxxxxx>> *On
> >> Behalf Of *Ian McInerney
> >> *Sent:* 12 January 2020 12:24
> >> *To:* Jeff Young <jeff@xxxxxxxxx <mailto:jeff@xxxxxxxxx>>
> >> *Cc:* KiCad Developers <kicad-developers@xxxxxxxxxxxxxxxxxxx
> >> <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>>
> >> *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 <jeff@xxxxxxxxx
> >> <mailto:jeff@xxxxxxxxx>> wrote:
> >>> We could also just require them on if/then/else statements….
> >>>
> >>>
> >>>> On 12 Jan 2020, at 00:35, Jeff Young <jeff@xxxxxxxxx
> >>>> <mailto:jeff@xxxxxxxxx>> 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 <stambaughw@xxxxxxxxx
> >>>>> <mailto:stambaughw@xxxxxxxxx>> 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 <seth@xxxxxxxxxxxxx
> >>>>>> <mailto:seth@xxxxxxxxxxxxx>
> >>>>>> <mailto:seth@xxxxxxxxxxxxx>> 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 <jeff@xxxxxxxxx
> >>>>>> <mailto:jeff@xxxxxxxxx>
> >>>>>>    <mailto:jeff@xxxxxxxxx>> 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@xxxxxxxxxxxxxxxxxxx
> >>>>>> <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> >>>>>>    <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> >>>>>>    Unsubscribe : https://launchpad.net/~kicad-developers
> >>>>>>    More help   : https://help.launchpad.net/ListHelp
> >>>>>>
> >>>>>>
> >>>>>> _______________________________________________
> >>>>>> Mailing list: https://launchpad.net/~kicad-developers
> >>>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> >>>>>> <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> >>>>>> Unsubscribe : https://launchpad.net/~kicad-developers
> >>>>>> More help   : https://help.launchpad.net/ListHelp
> >>>>>>
> >>>>>
> >>>>> _______________________________________________
> >>>>> Mailing list: https://launchpad.net/~kicad-developers
> >>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> >>>>> <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> >>>>> Unsubscribe : https://launchpad.net/~kicad-developers
> >>>>> More help   : https://help.launchpad.net/ListHelp
> >>>>
> >>>> _______________________________________________
> >>>> Mailing list: https://launchpad.net/~kicad-developers
> >>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> >>>> <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> >>>> Unsubscribe : https://launchpad.net/~kicad-developers
> >>>> More help   : https://help.launchpad.net/ListHelp
> >>>
> >>> _______________________________________________
> >>> Mailing list: https://launchpad.net/~kicad-developers
> >>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> >>> <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> >>> Unsubscribe : https://launchpad.net/~kicad-developers
> >>> More help   : https://help.launchpad.net/ListHelp
> >> _______________________________________________
> >> Mailing list: https://launchpad.net/~kicad-developers
> >> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> >> <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> >> Unsubscribe : https://launchpad.net/~kicad-developers
> >> More help   : https://help.launchpad.net/ListHelp
> >
> >
> > _______________________________________________
> > Mailing list: https://launchpad.net/~kicad-developers
> > Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> > Unsubscribe : https://launchpad.net/~kicad-developers
> > More help   : https://help.launchpad.net/ListHelp
> >
>
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>