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