1649112 - nsXPConnect.cpp: using the result of an assignment as a condition without parentheses

Status: RESOLVED WONTFIX

(Core :: XPConnect, task)

Description

[Sylvestre Ledru [:Sylvestre]]

https://searchfox.org/mozilla-central/source/js/xpconnect/src/nsXPConnect.cpp#406-411

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1649112 - nsXPConnect.cpp: add parentheses around the assignment to silence this warning r?jandem

Comment 2

[Tom S. (please needinfo tschuster)]

This seems wrong to me. This specific syntax should not require braces, because it includes a type. Is this a new kind of warning introduced by clang?

Comment 3

[Sylvestre Ledru [:Sylvestre]]

No, it has been around for a while: https://clang.llvm.org/docs/DiagnosticsReference.html#widiomatic-parentheses
It is just best practice as it clearly highlight to humans that it is really what we want to do and not a typo.

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Attachment: what we see in our private instance

Comment 5

[Tom S. (please needinfo tschuster)]

Very strange. I thought this syntax was recommended, because unlike if (priv = xpc::CompartmentPrivate::Get(obj)) you can't directly confuse that with ==.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Sylvestre Ledru [:Sylvestre] from comment #3)

No, it has been around for a while: https://clang.llvm.org/docs/DiagnosticsReference.html#widiomatic-parentheses
It is just best practice as it clearly highlight to humans that it is really what we want to do and not a typo.

I agree in the general case, but I agree with Tom that this is a special pattern that we use in hundreds of places without parentheses. Here are some results:

https://searchfox.org/mozilla-central/search?q=if+%5C%28dom%3A%3A.*%5B%5E%21%5D%3D%5B%5E%3D%5D&path=&case=false&regexp=true
https://searchfox.org/mozilla-central/search?q=if+%5C%28mozilla%3A%3A.*%5B%5E%21%5D%3D%5B%5E%3D%5D&path=&case=false&regexp=true
https://searchfox.org/mozilla-central/search?q=if+%5C%28js%3A%3A.*%5B%5E%21%5D%3D%5B%5E%3D%5D&path=&case=false&regexp=true

Comment 7

[Jan de Mooij [:jandem]]

It also doesn't compile with the parentheses I think?

Comment 8

[Sylvestre Ledru [:Sylvestre]]

I guess it is an glitch from the CI. It is the only occurrence found by the tool.