1139547 - nsWildCard.cpp:332:22: error: unsequenced modification and access to 'y' [-Werror,-Wunsequenced]

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

A try push of mine [1] hit a build failure on OS X builds only. The compiler (I think correctly) pointed out unsequenced operations on a variable which can result in compiler-dependent final behaviour. The puzzling bit is that this error reproduced on my patches which have nothing to do with this code at all. I can reproduce this locally, and the specific change that causes this error to show up is the one at [2].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddc93861714d
[2] https://hg.mozilla.org/try/rev/86500395a2db

Comment 1

[Kartikaya Gupta (email:[e-mail])]

I bisected my way through the code and the specific change that causes this error to show up is that the line of code at [1] now gets included into nsWildCard.cpp (because in my change I #included nsIWidget.h into IPCMessageUtils.h), and that's what makes the error happen.

Nathan, thoughts? I'm happy to just fix nsWildCard.cpp but I hope there isn't something more serious lurking here.

[1] http://mxr.mozilla.org/mozilla-central/source/mfbt/TypedEnumBits.h?rev=7f7f003696ad#90

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Also ni? Waldo since Nathan is away this week and this might be relevant to MFBT.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Comment 4

[Botond Ballo [:botond]]

Interesting. Some observations:

  - The line in TypedEnumBits.h defines some 'operator &&' overloads at namespace scope.

  - The line in nsWildCard.cpp contains an '&&' expression, with the two modifications 
    to 'y' being in either operand.

  - A call to the built-in 'operator &&' has short-circuit semantics, which means the
    evaluation of the first operand must be sequenced before the evaluation of the
    second.

  - A call to an overloaded 'operator &&', on the other hand, is just like any other
    function call, with the evaluations of the arguments being unsequenced.

I wonder, therefore, if this warning suggests that one of the 'operator &&' overloads is now being preferred over the built-in 'operator &&' in the line in nsWildCard.cpp. I don't see any reason why it would be (and even if it was, it would be harmless because the overloaded 'operator &&' has the same semantics as the built-in one), but that would explain the warning.

Another possibility is that for some reason, just having an overloaded 'operator &&' as a _candidate_, without it being chosen, triggers the warning.

Comment 5

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #4)
> Another possibility is that for some reason, just having an overloaded
> 'operator &&' as a _candidate_, without it being chosen, triggers the
> warning.

Ah, I see what's happening.

  - The line in nsWildCard.cpp is inside a function template, and the
    types of operands to the '&&' expression are dependent on the template
    parameters.

  - The warning does not contain a template instantiation backtrace,
    suggesting that the warning analysis is being performed on the
    uninstantiated template code, rather than a particular
    instantiation.

  - Therefore, the warning analysis doesn't know what the concrete types
    of the operands to the '&&' are, and therefore must assume that for
    some instantiations, the overloaded 'operator &&' could be chosen.

Conclusions:

  - There is no serious issue lurking around.

  - Don't write code that modifies the same variable multiple times
    in the same statement :)

Comment 6

[Botond Ballo [:botond]]

Comment on attachment 8572784 [details] [diff] [review]
Patch

Review of attachment 8572784 [details] [diff] [review]:
-----------------------------------------------------------------

Drive-by r+

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8572784 [details] [diff] [review]
Patch

Thanks! I'd still like to get an official XPCOM reviewer to review this even though it's a trivial patch.

Comment 8

[Benjamin Smedberg]

Comment on attachment 8572784 [details] [diff] [review]
Patch

Good god.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d1fca446b7e

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1d1fca446b7e