|
|
|
Fix trailing commas in enums
R=reed1
BUG=
TEST=
Patch Set 1 #Patch Set 2 : Add -pedantic to CXXFLAGS and update all enums #
Total comments: 1
Total messages: 13
|
gw280
|
13 years, 5 months ago (2012年07月19日 22:04:47 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Committed revision 4686. Can you update the .gyp files so that we see this warning on our bots?
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.
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> > . > >
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.
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.
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.
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?
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?
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.
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 ','.
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.
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...