Re-enable the clang check for enum constants.

Re-enable the clang check for enum constants.

The patch adds a flag to disable the feature, so as to avoid future reverts.
There is also a check to avoid mixed signed comparisons, as can
arise with enums from C files.  We need not bother checking these.

BUG=356816
R=mark@chromium.org
R=thakis@chromium.org

Re-enable flag.


patch from issue 150943005

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

[Tom Sepez, 2014-03-27 21:22:44]

Mark, Nico, please review.

[Nico, 2014-03-27 21:32:33]

Please also add tests. See the other tests in that folder for examples.

[Tom Sepez, 2014-03-27 21:59:19]

> Please also add tests. See the other tests in that folder for examples.
Tests added.  Thanks for the pointer to the examples.

[Nico, 2014-03-27 22:53:45]

mostly lgtm, thanks!

https://codereview.chromium.org/213403010/diff/40001/tools/clang/plugins/Chro...
File tools/clang/plugins/ChromeClassTester.cpp (right):

https://codereview.chromium.org/213403010/diff/40001/tools/clang/plugins/Chro...
tools/clang/plugins/ChromeClassTester.cpp:209:
ignored_record_names_.insert("ServerFieldType");
Hm, maybe the warning should only fire if all enum values have a common prefix,
and the last entry is prefix_LAST?

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

https://codereview.chromium.org/213403010/diff/40001/tools/clang/plugins/Find...
tools/clang/plugins/FindBadConstructs.cpp:759: } else if (args[i] ==
"skip-enum-last-value") {
Usually, new warnings default to off, and are turned on after a clang roll has
safely landed and been in the tree for a while, to make the roll easier.

[Nico, 2014-03-27 22:56:49]

Oh, and since that was a problem, consider having a .c test file maybe

[Tom Sepez, 2014-03-27 23:06:33]

On 2014/03/27 22:56:49, Nico wrote:
> Oh, and since that was a problem, consider having a .c test file maybe
Will do.  Was hoping to avoid mucking with the test.sh itself, but sure.

[Tom Sepez, 2014-03-27 23:09:38]

> Hm, maybe the warning should only fire if all enum values have a common
prefix,
> and the last entry is prefix_LAST?
> 
Although I did it that way in the test, its not always the case.  The naming
need not be consistent:

enum Color {
  RED,
  GREEN,
  BLUE,
  COLOR_LAST = BLUE,
};

and a common botch is adding onto the enum 

enum Color {
  RED,
  GREEN,
  BLUE,
  COLOR_LAST = BLUE,
  PINK
};

>
https://codereview.chromium.org/213403010/diff/40001/tools/clang/plugins/Find...
> File tools/clang/plugins/FindBadConstructs.cpp (right):
> 
>
https://codereview.chromium.org/213403010/diff/40001/tools/clang/plugins/Find...
> tools/clang/plugins/FindBadConstructs.cpp:759: } else if (args[i] ==
> "skip-enum-last-value") {
> Usually, new warnings default to off, and are turned on after a clang roll has
> safely landed and been in the tree for a while, to make the roll easier.
Will do.

[Tom Sepez, 2014-03-28 01:08:11]

Nico, please take a final look. Thanks.

[Nico, 2014-03-28 03:31:50]

lgtm!

https://codereview.chromium.org/213403010/diff/60001/tools/clang/plugins/test...
File tools/clang/plugins/tests/enum_last_value_from_c.c (right):

https://codereview.chromium.org/213403010/diff/60001/tools/clang/plugins/test...
tools/clang/plugins/tests/enum_last_value_from_c.c:24: // We dont warn when
xxxLAST constants are last.
don't

https://codereview.chromium.org/213403010/diff/60001/tools/clang/plugins/test...
tools/clang/plugins/tests/enum_last_value_from_c.c:33: // We dont warn when
xxx_LAST constants are last.
don't

[Tom Sepez, 2014-03-28 17:39:35]

The CQ bit was checked by tsepez@chromium.org

[commit-bot: I haz the power, 2014-03-28 17:41:56]

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

[commit-bot: I haz the power, 2014-03-28 20:05:16]

Change committed as 260249

[Nico, 2015-05-10 18:48:42]

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

https://codereview.chromium.org/213403010/diff/80001/tools/clang/plugins/Find...
tools/clang/plugins/FindBadConstructs.cpp:762: options_.check_enum_last_value =
true;
It looks like this check is off by default and the flag isn't set. Should be bug
for this really be marked fixed? Or am I missing something?

[Tom Sepez, 2015-05-11 18:02:25]

> It looks like this check is off by default and the flag isn't set. Should be
bug
> for this really be marked fixed? Or am I missing something?
Bug should probably be marked wontfix, as we found this was undesirable in
general.

[Nico, 2015-05-11 18:04:59]

Should this code be deleted from the plugin then?

On Mon, May 11, 2015 at 11:02 AM, <tsepez@chromium.org> wrote:

> Reviewers: Mark Mentovai, Nico,
>
> Message:
>
>> It looks like this check is off by default and the flag isn't set. Should
>> be
>>
> bug
>
>> for this really be marked fixed? Or am I missing something?
>>
> Bug should probably be marked wontfix, as we found this was undesirable in
> general.
>
>
> Description:
> Re-enable the clang check for enum constants.
>
> The patch adds a flag to disable the feature, so as to avoid future
> reverts.
> There is also a check to avoid mixed signed comparisons, as can
> arise with enums from C files.  We need not bother checking these.
>
> BUG=356816
> R=mark@chromium.org
> R=thakis@chromium.org
>
> Re-enable flag.
>
>
> patch from issue 150943005
>
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260249
>
> Please review this at https://codereview.chromium.org/213403010/
>
> Base URL: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+172, -1 lines):
>   M tools/clang/plugins/ChromeClassTester.h
>   M tools/clang/plugins/ChromeClassTester.cpp
>   M tools/clang/plugins/FindBadConstructs.cpp
>   A tools/clang/plugins/tests/enum_last_value.cpp
>   A tools/clang/plugins/tests/enum_last_value.flags
>   A tools/clang/plugins/tests/enum_last_value.txt
>   A tools/clang/plugins/tests/enum_last_value_from_c.c
>   A tools/clang/plugins/tests/enum_last_value_from_c.flags
>   A tools/clang/plugins/tests/enum_last_value_from_c.txt
>   M tools/clang/plugins/tests/test.sh
>
>
>

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