Add a check to the FindBadConstructs.cpp clang plugin for bad enum last values.

Add a check to the FindBadConstructs.cpp clang plugin for bad enum last values.

One common coding patern is to define an enum as in:
  enum Color { RED, GREEN, BLUE, COLOR_LAST=BLUE };
but this is fragile when someone adds a new constant and forgets
to update the COLOR_LAST constant.

This change looks for enums that have a xxx_LAST or xxxLast member,
and warns if there are any higher-valued constants present.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251894

[Tom Sepez, 2014-02-11 22:10:06]

erg@, please review.

[darin (slow to review), 2014-02-12 05:59:46]

LGTM, but I am not an owner.

https://codereview.chromium.org/150943005/diff/50001/tools/clang/plugins/Find...
File tools/clang/plugins/FindBadConstructs.cpp (right):

https://codereview.chromium.org/150943005/diff/50001/tools/clang/plugins/Find...
tools/clang/plugins/FindBadConstructs.cpp:157: if
(options_.check_bad_enum_last_value) {
nit: I recommend returning early if this option is not set. That way you get to
reduce indentation for the rest of the function.

[Elliot Glaysher, 2014-02-12 20:27:16]

Is this actually a common pattern? I've never seen it in chrome code, though I
have seen terminators which have their own entry like this:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/layout.h&t...

to support the for (int i = 0; i < BLAH_LAST; ++i) pattern.

[Tom Sepez, 2014-02-12 22:50:04]

Elliot, I'll forward you the (lengthy) thread where we discussed that this is a
reasonable pattern.

[Elliot Glaysher, 2014-02-13 18:27:32]

I think that I only got a part of that thread, but I think that I got the
jist:

There's a desire to turn on the warning for unhandled enums in switch
statements. Because it's a common pattern that we have _MAX in our code,
this is preventing us from turning it on since we'd then have to have a
case _MAX: in every switch statement that handles an enum. However, if two
of the enums have equivalent values, one of them can be omitted, so we're
trying to change the pattern to _LAST (and just change all the loops to
<=). However, since that's kind of fragile, (since the _LAST = x part can
get out of date), you want to add tooling to check to make sure that _LAST
always equals the last element of the enum.

Is that a fair summary? If it is, then this patch LGTM.


On Wed, Feb 12, 2014 at 2:50 PM, <tsepez@chromium.org> wrote:

> Elliot, I'll forward you the (lengthy) thread where we discussed that this
> is a
> reasonable pattern.
>
> https://codereview.chromium.org/150943005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Tom Sepez, 2014-02-13 19:10:27]

> There's a desire to turn on the warning for unhandled enums in switch
> statements. Because it's a common pattern that we have _MAX in our code,
> this is preventing us from turning it on since we'd then have to have a
> case _MAX: in every switch statement that handles an enum. However, if two
> of the enums have equivalent values, one of them can be omitted, so we're
> trying to change the pattern to _LAST (and just change all the loops to
> <=). However, since that's kind of fragile, (since the _LAST = x part can
> get out of date), you want to add tooling to check to make sure that _LAST
> always equals the last element of the enum.
> 
> Is that a fair summary? If it is, then this patch LGTM.
Exactly.

I had a question about my "check-bad-enum-last-value" switch: is this strictly
required, or is this only for cases where the codebase isn't presently clean
with respect to the check? I also just realized that I hard-coded this "on"
since I didn't spend the time to figure out how to plump this flag all the way
through via GYP_DEFINES ...

[Tom Sepez, 2014-02-18 22:02:59]

patch set 4 removes the flag.

[Tom Sepez, 2014-02-18 22:03:32]

The CQ bit was checked by tsepez@chromium.org

[commit-bot: I haz the power, 2014-02-18 22:09:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/150943005/180001

[commit-bot: I haz the power, 2014-02-19 00:20:21]

Change committed as 251894

[Nico, 2014-03-26 20:05:58]

Please ALWAYS land new warnings behind a flag, so that if they don't work well
they don't jeopardize clang rolls.

(Plugin reviewers: Please always check for this.)

Looks like this is causing issues :-/

[Tom Sepez, 2014-03-26 20:10:14]

On 2014/03/26 20:05:58, Nico wrote:
> Please ALWAYS land new warnings behind a flag, so that if they don't work well
> they don't jeopardize clang rolls.
Ah. I'd asked about this in c5, now we have our answer.

> Looks like this is causing issues :-/
Do tell.

[Nico, 2014-03-26 20:15:20]

On Wed, Mar 26, 2014 at 1:10 PM, <tsepez@chromium.org> wrote:

> On 2014/03/26 20:05:58, Nico wrote:
>
>> Please ALWAYS land new warnings behind a flag, so that if they don't work
>> well
>> they don't jeopardize clang rolls.
>>
> Ah. I'd asked about this in c5, now we have our answer.
>
>
>  Looks like this is causing issues :-/
>>
> Do tell.
>

Bugs are being filed, you'll see them soon. It warns on system headers for
rsesek, and makes the compiler crash. I'll likely revert this for now and
land an oilpan plugin refresh with the revert tomorrow. That'll remove this
check too, so that we don't need to revert the clang roll.


>
> https://codereview.chromium.org/150943005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-03-26 20:44:16]

A revert of this CL has been created in
https://codereview.chromium.org/212673008/ by thakis@chromium.org.

The reason for reverting is: Caused crbug.com/356815 and crbug.com/356816 and
doesn't have a flag to turn it off..