Re: Enable -Wswitch-enum? [was Re: MOZ_ASSUME_UNREACHABLE is being misused]
(2014/04/07 14:27), Karl Tomlinson wrote:
It is allowed in N3242. I think the relevant sections are
5.2.9 Static cast
Thank you for the pointer.
I found a floating copy of n3242.pdf at the following url.
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3242.pdf
I think 7.2 10 is also relevant here.
--- quote ---
An expression of arithmetic or enumeration type can be converted to an
enumeration type explicitly. The
value is unchanged if it is in the range of enumeration values of the
enumeration type; otherwise the resulting
enumeration value is unspecified.
--- end quote
I take so :
typedef enum { a = 1, b, c = 10 } T;
T x;
x = 3; /* is OK it is within [1..10] although 3 is not in the list
explicitly */
but
x = 32; /*`unspecified' because it is outside [1..2^16] */
(I read the specification to mean
that the range of enumeration values is
[0 or 1 .. maximum_necessary_for_the_declared_maximum_value]
where maximum_necessary_for_the_declared_maximum_value is
the 2^M or (2^M-1) etc. depending on how the
negative value is represented. 2's complement or 1's complement.)
The description is very complex, but
it seems that the enumeration range be
calculated to produce the narrowest bit field that can contain
all the explicitly declared values (min .. max)
This is the range of values.
Anyway, in my example above, a compiler can do anything
if x = 32 is executed (?).
Hmm.I will re-read 7.2.7.
TIA
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Enable -Wswitch-enum? [was Re: MOZ_ASSUME_UNREACHABLE is being misused]
chiaki ISHIKAWA writes:
I think 7.2 10 is also relevant here.
--- quote ---
An expression of arithmetic or enumeration type can be converted
to an enumeration type explicitly. The
value is unchanged if it is in the range of enumeration values of
the enumeration type; otherwise the resulting
enumeration value is unspecified.
--- end quote
I take so :
typedef enum { a = 1, b, c = 10 } T;
T x;
Anyway, in my example above, a compiler can do anything
if x = 32 is executed (?).
Note here the enumeration value is unspecified, which I assume
merely means the compiler can choose anything for the value of x.
That might be considerably safer than undefined behavior of the
program.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Enable -Wswitch-enum? [was Re: MOZ_ASSUME_UNREACHABLE is being misused]
On 2014-04-07 6:00 AM, Karl Tomlinson wrote:
chiaki ISHIKAWA writes:
I think 7.2 10 is also relevant here.
--- quote ---
An expression of arithmetic or enumeration type can be converted
to an enumeration type explicitly. The
value is unchanged if it is in the range of enumeration values of
the enumeration type; otherwise the resulting
enumeration value is unspecified.
--- end quote
I take so :
typedef enum { a = 1, b, c = 10 } T;
T x;
Anyway, in my example above, a compiler can do anything
if x = 32 is executed (?).
Note here the enumeration value is unspecified, which I assume
merely means the compiler can choose anything for the value of x.
That might be considerably safer than undefined behavior of the
program.
Right. The intention here is that the compiler is allowed to pick some
underlying integer type, which must be able to represent all declared
values of the enumeration, and then silently accept in-range values and
truncate out-of-range values *for that type* -- but it's *not* allowed
to apply assume the programmer never does that optimizations as it
would for undefined behavior.
I don't know what the C++ committee's attitude was, but the C committee
specifically wanted to allow use of `enum` to declare bitmasks:
enum X {
X_FOO = 0x0001,
X_BAR = 0x0002,
/* ... exhaustive list of bit flags here ... */
X_BLURF = 0x8000
};
For that use case, any _combination_ of X_* flags must be representable
by the underlying integer type; note that many such combinations are
outside the *declared* range of values.
zw
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Enable -Wswitch-enum? [was Re: MOZ_ASSUME_UNREACHABLE is being misused]
On Fri, 04 Apr 2014 11:03:57 -0400, Zack Weinberg wrote: On Tue, Apr 1, 2014 at 10:13 PM, Karl Tomlinson mozn...@karlt.net wrote: Does WARNINGS_AS_ERRORS make the default:MOZ_CRASH() unnecessary? No, because it's possible that the thing you're testing is not actually a valid enum value, such as if it was incorrectly cast. Our codebase has no shortage of dubious casts. Also, in many cases default:MOZ_CRASH() would kill control flow paths that make the compiler think variables might be used uninitialized. OK. Even with -Wswitch issuing no warning with no default clause, the compiler may need to handle the switch matching no case, because enumeration types may hold values that don't match any of their enumerator values. However, if the compiler knows that falling through would cause a variable to be used uninitialized, then it knows that would have undefined behaviour, so it would not need to handle that situation in any particular way. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Enable -Wswitch-enum? [was Re: MOZ_ASSUME_UNREACHABLE is being misused]
(2014/04/07 10:16), Karl Tomlinson wrote: because enumeration types may hold values that don't match any of their enumerator values. Is this allowed by C (or C++) specification today? [Yes, I know the compiler in the past did not care much.] I thought the stricter warnings of compilers today is that if something like that happens, anything goes in principle (because of the spec allows the compiler to produce such code.) Maybe I was mistaken. TIA ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Enable -Wswitch-enum? [was Re: MOZ_ASSUME_UNREACHABLE is being misused]
chiaki ISHIKAWA writes: (2014/04/07 10:16), Karl Tomlinson wrote: because enumeration types may hold values that don't match any of their enumerator values. Is this allowed by C (or C++) specification today? It is allowed in N3242. I think the relevant sections are 5.2.9 Static cast 10 A value of integral or enumeration type can be explicitly converted to an enumeration type. The value is unchanged if the original value is within the range of the enumeration values (7.2). Otherwise, the resulting enumeration value is unspecified. 7.2 Enumeration declarations 7 For an enumeration whose underlying type is fixed, the values of the enumeration are the values of the underlying type. Otherwise, for an enumeration where e min is the smallest enumerator and e max is the largest, the values of the enumeration are the values in the range b min to b max , defined as follows: Let K be 1 for a two’s complement representation and 0 for a one’s complement or sign-magnitude representation. b max is the smallest value greater than or equal to max(|e min | − K, |e max |) and equal to 2 M − 1, where M is a non-negative integer. b min is zero if e min is non-negative and −(b max + K) otherwise. The size of the smallest bit-field large enough to hold all the values of the enumeration type is max(M, 1) if b min is zero and M + 1 otherwise. It is possible to define an enumeration that has values not defined by any of its enumerators. If the enumerator-list is empty, the values of the enumeration are as if the enumeration had a single enumerator with value 0. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Enable -Wswitch-enum? [was Re: MOZ_ASSUME_UNREACHABLE is being misused]
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 04/02/2014 07:37 AM, Aryeh Gregor wrote: On Tue, Apr 1, 2014 at 5:56 PM, Zack Weinberg za...@panix.com wrote: The downside of turning this on would be that any switch statements that *deliberately* include only a subset of the enumerators, plus a default case, would now have to be expanded to cover all the enumerators. If there are few enough that this is sane. There are at least a few switches on nsresult in the tree, IIRC. Those would have to be rewritten as if/else. I think we have some other big enums lying around, although probably not so dramatic. Given the numbers posted elsewhere, I think this is clearly not going anywhere until someone has time and working development machine to find out just how burdensome it is. I have no idea how long it will be before I personally can do such experiments, alas. On Tue, Apr 1, 2014 at 10:13 PM, Karl Tomlinson mozn...@karlt.net wrote: Does WARNINGS_AS_ERRORS make the default:MOZ_CRASH() unnecessary? No, because it's possible that the thing you're testing is not actually a valid enum value, such as if it was incorrectly cast. Our codebase has no shortage of dubious casts. Also, in many cases default:MOZ_CRASH() would kill control flow paths that make the compiler think variables might be used uninitialized. zw -BEGIN PGP SIGNATURE- Version: GnuPG v1 Comment: Using GnuPG with Icedove - http://www.enigmail.net/ iQIcBAEBCAAGBQJTPsnUAAoJEJH8wytnaapkyRgQALgWSAFa+FLLyYuFeuQis9zs KhqmNCYmCtTk9KNv8WhPTZCN6QYEekKPGKyoi5A+DXaRPqkqf6N2X3F8bWQbIrTB hZeRKm7Ks9TEvrBWQy8NwGV0Q2BftWpvvoKlHu44/Z8A1o8bW9X3kA/4GRltCi6+ jizXprEQ96opIOA2/7X7JdOG4jMnyNgeX236cgTQOIzf1RF2at70OGfVOZ+FcnPO UjctNx32L6xd306SIzWI/gwGXP7e9rxVl87/vjk+R8HcBoNNftkphRoZuP1L7h9E QNAAwdYUUa5TWrku9jeYIKgzI+gFTt2IjTsglIx3tHSlhB9Eigni6FzqYPjmUjQ+ sRn+726/cVDu8o9iKyZ3vTAxEYHHGFQVTLozlHp3zaVjqu02k6QvCZ4qkBJxU+0D zr3QZFg4WgN0FnS15b8HeeJKXfE1x8FQWsT7lVITlE7+kNq3DgHK7REpJvwxvGFX L6IdTY2Ag1V3ytywWgY8Qh1Tjvf73j+tMcO9Ka2auyGDJzwwem1BpfHJiCfQ4So4 SCJ/xH65p7Twgh7L//gIwMo2lhovP3/YTel5yVXSe5yIAzfoJ3mIMAKmF6vkcI55 ytD48TyNCxCCtEndc3QnsGA5EvRnM1tkAb01dfEVmrVmruFXoR6cTjJ3yiIgU3V3 OXTT6fwLxm/4bqUEwDcp =TW5u -END PGP SIGNATURE- ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Enable -Wswitch-enum? [was Re: MOZ_ASSUME_UNREACHABLE is being misused]
On Tue, Apr 1, 2014 at 5:56 PM, Zack Weinberg za...@panix.com wrote: The downside of turning this on would be that any switch statements that *deliberately* include only a subset of the enumerators, plus a default case, would now have to be expanded to cover all the enumerators. If there are few enough that this is sane. There are at least a few switches on nsresult in the tree, IIRC. Those would have to be rewritten as if/else. I think we have some other big enums lying around, although probably not so dramatic. On Tue, Apr 1, 2014 at 10:13 PM, Karl Tomlinson mozn...@karlt.net wrote: Does WARNINGS_AS_ERRORS make the default:MOZ_CRASH() unnecessary? No, because it's possible that the thing you're testing is not actually a valid enum value, such as if it was incorrectly cast. Our codebase has no shortage of dubious casts. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Enable -Wswitch-enum? [was Re: MOZ_ASSUME_UNREACHABLE is being misused]
On 04/01/2014 08:56 AM, Zack Weinberg wrote: The downside of turning this on would be that any switch statements that *deliberately* include only a subset of the enumerators, plus a default case, would now have to be expanded to cover all the enumerators. I would try it and report on how unpleasant that turns out to be, but my development box has taken to corrupting its filesystem if I merely do a 'hg update', so I'm kind of stuck, there. Anyone interested in trying it? As a first approximation, it seems like we'd have to do this for a most switch statements that include a 'default:' case. (I'm betting that most of those switch statements have omitted *some* enum values, since until now, there's been no reason to include 'default' unless you're trying to be concise and merge some values together.) grep finds 3934 default statements in .cpp files, 469 in .h files: $ grep -r default: | grep .cpp: | wc -l 3934 $ grep -r default: | grep .h: | wc -l 469 So, we have on the order of ~4400 switch statements that would potentially need expanding to avoid tripping this warning. Having said that, if we *really* wanted to add this warning, I suppose we could add it directory-by-directory (or only ever add it in certain directories, as is the case with -Wshadow in layout/style). ~Daniel ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Enable -Wswitch-enum? [was Re: MOZ_ASSUME_UNREACHABLE is being misused]
Zack Weinberg writes: This is a bit of a tangent, but: There are a whole bunch of places in layout, and probably elsewhere, where we have the following catch-22: if you have a switch statement over the values of an enumeration, gcc and clang (with -Wall) will warn you about enumeration values that are not handled, but only if there is no default: clause. In basically all of the instances I know about, default: MOZ_CRASH() would be appropriate, but has been omitted in the name of continuing to get that warning (many of these enumerations do regularly get new values added). Does WARNINGS_AS_ERRORS make the default:MOZ_CRASH() unnecessary? My initial impression from cairo code, which uses -Wswitch-enum, is that it can be burdensome, so an optional solution, such as choosing whether to include the default clause, sounds appealing. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Enable -Wswitch-enum? [was Re: MOZ_ASSUME_UNREACHABLE is being misused]
On 4/1/14, 10:22 AM, Daniel Holbert wrote: So, we have on the order of ~4400 switch statements that would potentially need expanding to avoid tripping this warning. clang on OS X reports 1635 -Wswitch-enum warnings (switch on enum not handling all enum cases). gcc reports 1048 -Wswitch-default warnings (switch without default case). clang seems to silently ignore gcc's -Wswitch-default flag. chris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
