kicad-developers team mailing list archive

[Jeff Young <jeff@xxxxxxxxx>]

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> 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> 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 <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 <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 <https://launchpad.net/~kicad-developers>
>>>>>    Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>>>>    <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx <mailto: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 <https://launchpad.net/~kicad-developers>
>>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx <mailto: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 <https://launchpad.net/~kicad-developers>
>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx <mailto: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 <https://launchpad.net/~kicad-developers>
>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx <mailto: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 <https://launchpad.net/~kicad-developers>
>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx <mailto: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 <https://launchpad.net/~kicad-developers>
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx <mailto: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>