Fix trailing commas in enums

Fix trailing commas in enums


R=reed1
BUG=
TEST=

[reed1, 2012-07-20 11:36:31]

Committed revision 4686.

Can you update the .gyp files so that we see this warning on our bots?

[bungeman, 2012-07-20 14:29:46]

On the one hand I understand this change, the trailing comma in enums is not
allowed in C89 or C++98. However, it is supported in C99 and C++11, and all
current compilers support it (and have supported it as an extension for quite
some time). I would rather propose that this trailing comma in enum be the first
C++11 feature Skia declares supported, unless there is a good reason to be
pedantic.

[TomH, 2012-07-20 14:55:20]

We actually have external users still on compilers that don't permit it. (I
can't remember if this was Android x86, or one of the Linux distros, or ???)


On Fri, Jul 20, 2012 at 10:29 AM, <bungeman@google.com> wrote:

> On the one hand I understand this change, the trailing comma in enums is
> not allowed in C89 or C++98. However, it is supported in C99 and C++11,
> and all current compilers support it (and have supported it as an
> extension for quite some time). I would rather propose that this
> trailing comma in enum be the first C++11 feature Skia declares
> supported, unless there is a good reason to be pedantic.
>
>
>
https://codereview.appspot.**com/6427056/<https://codereview.appspot.com/6427...
>
> --
> You received this message because you are subscribed to the Google Groups
> "skia-review" group.
> To post to this group, send email to skia-review@googlegroups.com.
> To unsubscribe from this group, send email to skia-review+unsubscribe@**
> googlegroups.com <skia-review%2Bunsubscribe@googlegroups.com>.
> For more options, visit this group at http://groups.google.com/**
> group/skia-review?hl=en <http://groups.google.com/group/skia-review?hl=en>
> .
>
>

[gw280, 2012-07-20 15:07:09]

On 2012/07/20 14:55:20, TomH wrote:
> We actually have external users still on compilers that don't permit it. (I
> can't remember if this was Android x86, or one of the Linux distros, or ???)
> 

It seems to me that while there are no advantages to supporting this, there are
disadvantages in that it limits the number of compilers Skia would support
without warnings.

[bungeman, 2012-07-20 15:57:30]

If we really want this we would need to do all of them, add the rule to the
style guide, and modify out gyps to ensure this warning is an error. Note that
we do this in a lot of places, run 

rgrep --exclude-dir "*.svn" --exclude-dir "out" -Pzo
"enum[^{]*{[^}]*,\s*(//[^\n]*)?[ \n]*}" .

from the top level of a Skia checkout to get the list. To see the list of files
run

rgrep --exclude-dir "*.svn" --exclude-dir "out" -Pzoc
"enum[^{]*{[^}]*,\s*(//[^\n]*)?[ \n]*}" . | grep -v :0

Also, src/sfnt/SkTypedEnum.h would need to be updated.

[gw280, 2012-07-20 18:26:17]

On 2012/07/20 15:57:30, bungeman wrote:
> If we really want this we would need to do all of them, add the rule to the
> style guide, and modify out gyps to ensure this warning is an error. Note that
> we do this in a lot of places, run 
> 
> rgrep --exclude-dir "*.svn" --exclude-dir "out" -Pzo
> "enum[^{]*{[^}]*,\s*(//[^\n]*)?[ \n]*}" .
> 
> from the top level of a Skia checkout to get the list. To see the list of
files
> run
> 
> rgrep --exclude-dir "*.svn" --exclude-dir "out" -Pzoc
> "enum[^{]*{[^}]*,\s*(//[^\n]*)?[ \n]*}" . | grep -v :0
> 
> Also, src/sfnt/SkTypedEnum.h would need to be updated.

So here's an updated patch that adds -pedantic to the CXXFLAGS and fixes all the
enums.

[TomH, 2012-07-20 18:27:45]

Over a discussion at lunch, multiple Skia devs objected to turning on -pedantic.
We should probably find a way to debate this more widely than a CL?

[gw280, 2012-07-20 18:33:16]

On 2012/07/20 18:27:45, TomH wrote:
> Over a discussion at lunch, multiple Skia devs objected to turning on
-pedantic.
> We should probably find a way to debate this more widely than a CL?

skia-discuss?

[bsalomon, 2012-07-20 18:50:49]

On 2012/07/20 18:33:16, gwright wrote:
> On 2012/07/20 18:27:45, TomH wrote:
> > Over a discussion at lunch, multiple Skia devs objected to turning on
> -pedantic.
> > We should probably find a way to debate this more widely than a CL?
> 
> skia-discuss?

skia-discuss is good. I'd like to know why this is necessary before seeing this
change committed.

[bungeman, 2012-07-20 19:43:53]

This CL (Patch Set 2) appears to be missing what would be a required change (as
mentioned above) to src/sfnt/SkTypedEnum.h, though it may not have been used on
Linux as of yet.

https://codereview.appspot.com/6427056/diff/7001/gyp/common_conditions.gypi
File gyp/common_conditions.gypi (right):

https://codereview.appspot.com/6427056/diff/7001/gyp/common_conditions.gypi#n...
gyp/common_conditions.gypi:105: '-pedantic'
Just because this is an appropriate time to use the word 'ironic', this line
should end with a ','.

[gw280, 2012-07-20 20:15:04]

On 2012/07/20 19:43:53, bungeman wrote:
> This CL (Patch Set 2) appears to be missing what would be a required change
(as
> mentioned above) to src/sfnt/SkTypedEnum.h, though it may not have been used
on
> Linux as of yet.

I will hold off on fixing this until we've reached a consensus on skia-dicuss as
to whether it's a good idea to fix the enums.

>
https://codereview.appspot.com/6427056/diff/7001/gyp/common_conditions.gypi#n...
> gyp/common_conditions.gypi:105: '-pedantic'
> Just because this is an appropriate time to use the word 'ironic', this line
> should end with a ','.

Haha :) I don't know gyp syntax and assumed that the trailing comma was not
allowed/necessary because previously there wasn't one.

[reed1, 2012-07-20 20:32:13]

my 2-cents:

fixing trailing commas seems useful it we're breaking a compiler somewhere,
independent (somewhat) if we want to turn-on pedantic warnings. I know we can
regress if we don't have pedantic turned on, but fixing now is a start. Hell, we
could try running Ben's script as part of our Housekeeping pass, to detect
regression w/o using the compiler...