kicad-developers team mailing list archive

["Bevan Weiss" <bevan.weiss@xxxxxxxxx>]

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> On Behalf Of Ian McInerney
Sent: 12 January 2020 12:24
To: Jeff Young <jeff@xxxxxxxxx>
Cc: KiCad Developers <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> https://launchpad.net/~kicad-developers
Post to     :  <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx> kicad-developers@xxxxxxxxxxxxxxxxxxx
Unsubscribe :  <https://launchpad.net/~kicad-developers> https://launchpad.net/~kicad-developers
More help   :  <https://help.launchpad.net/ListHelp> 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