kicad-developers team mailing list archive
-
kicad-developers team
-
Mailing list archive
-
Message #43232
Re: Coverity finds an ugly bug in wxWidgets
-
To:
kicad-developers@xxxxxxxxxxxxxxxxxxx
-
From:
Wayne Stambaugh <stambaughw@xxxxxxxxx>
-
Date:
Sun, 12 Jan 2020 07:36:46 -0500
-
Autocrypt:
addr=stambaughw@xxxxxxxxx; keydata= mQGiBEM0hxQRBAC2fNh3YOVLu1d5GZ0SbrTNldGiGnCJPLqzEnqFX9v6jmf33TMt6EmSLkl6 Wtfkoj0nVwKxcYmJkA8DX0QAokBkwNIzhSsBzQvthBLIk/5LnPVVKrEXOcL4mUyH1doKlkaE slgJozNa6Av+oavcvD02o1zJOloBbaHlNlyRt7fKswCgtIFlVjWggVH/15KfWk+Qo5JVPbME AIUBAQyL2OAx0n60AWec2WHnO9buHuG0ibtICgUMkE+2MRmYyKwYRdyVwGoIUemFuOyHp0AJ InX4T+vy2E7vkwODqjtMLfIoRkokW74Fi4nrvjlhOAw/vdq/twLbAmR9MOfPTpR4y7kQy1O2 /n+RkkRvh26vTzfbQmrH7cBJhk6aA/9Uwvu3E4zNJgHVZeS0HyWtmR1eOPPRbnkPgJTToX5O KMKzTJI/FX6kT7cFoCamitHrW3BJP4Dx+cMMsa47EGxqVTdbVJ4LjogsXTXxb+0Fn1u4zBdx x3Cer6O7+hqWy7zvpzeC6nSREjqDKa5CgHtv/GLm5uFPOmsjAsnHj2tlBrQmV2F5bmUgU3Rh bWJhdWdoIDxzdGFtYmF1Z2h3QGdtYWlsLmNvbT6IeAQTEQIAOBYhBOffs6CbblRzBkv33BtR cWlZ+CReBQJbFBS2AhsDBQsJCAcCBhUKCQgLAgQWAgMBAh4BAheAAAoJEBtRcWlZ+CReMI8A nRbrLkzp7+c2f0vX7sfg4ICX8LAKAJ9uClo4uJajmZa5zZrL2nKdZlUwIrkCDQRDNIcxEAgA gCru+3/aOC6RCjpvYC72wY+d5SmHphC6yeiV2/mOumyt5MLo/Ps2GznZr11JspqFk5K/Zpvp MMLqqjDZ39+50a2iKRQFJ6NlK+hJWMmj6eJygQrCwYo3Gjc6CqfrqUv+8VSnf/i5sIZmtOVA 4ZjML18MuBvMSsNdVLFJd5HNnYb1iOECpvqdPVh/21LLCEw7MUUGGnHBhCrmk2aJe5hFmcSN g4ldBcXrgMQBwf7aMVoobXBMFDb/IENByXn0llB7Gr2IFMRmNS9/p8s/II1Yl2bTqyX4FSz8 cfn7C9KEz7faZ7wzAcpwHFC/zs3JoAjJ0IEKdNUpIwAlKMzT3CzctwADBQf/cxpG28MKyrqk nNmq/8LQLy+x6FSYXBLjxQz9BiBNYeesDZQ6J5UbL1mjpJzMa5tLZypPYo4bbGyR22hrbyDF K7m6AcVaMIJKl98g4ukMutFfAJyRDaREH5Zl/X1P4u1Z/yaAIy9mKaNbaK1/5djNJ5wCTFen TUgAp9xdc30kGkFDdLJFp5uxDY4P0vaZiZdjUCvDM3Zjv5IzpNOfxVqTUBQNUP/BnnKhkk0p DTD6s3X8S+D0rOtEBQ8K0cwERI/E8EFa8nj0TNw4e2MYGR8wg+SxqJ7z5f0zPY0bO6G9DDFB wYCqzzPWGqdAh9vA5971TAbPERtdFybhkurozp2SfYhJBBgRAgAJBQJDNIcxAhsMAAoJEBtR cWlZ+CResHUAniULLCWiT26ieRTl7N2vS6vBo/DuAJ4m7Ss/gyiW6ybTn1ctDXAUgm2QVQ==
-
In-reply-to:
<9C505C3B-43FD-4A67-8111-B3F45C22FC5D@rokeby.ie>
-
Openpgp:
preference=signencrypt
-
User-agent:
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0
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
>
Follow ups
References