Explicit warning diagnostic flags for groups

Hi,

I'm new to this group, so forgive me if this is the wrong place.

I use clang with Xcode and like to run with all warnings (-Weverything -Werror) and then disable whatever warnings/enable whatever features I feel I can justify. Unfortunately, sometimes there are features like GNU's ?: expression extension, eliding middle term which do not have their own flags, so then the choice is either enable all the GNU extensions or do without the extension.

I've hit this in the past with clang, for things like vararg macros and such. So I'd be interested in seeing this remedied. I checked out the clang source and it appears that all that is needed to resolve this (for the eliding middle term case) is a simple patch like this:

Index: include/clang/Basic/DiagnosticGroups.td

Hi,

I'm new to this group, so forgive me if this is the wrong place.

I use clang with Xcode and like to run with all warnings (-Weverything -Werror) and then disable whatever warnings/enable whatever features I feel I can justify. Unfortunately, sometimes there are features like GNU's ?: expression extension, eliding middle term which do not have their own flags, so then the choice is either enable all the GNU extensions or do without the extension.

I've hit this in the past with clang, for things like vararg macros and such. So I'd be interested in seeing this remedied. I checked out the clang source and it appears that all that is needed to resolve this (for the eliding middle term case) is a simple patch like this:

Index: include/clang/Basic/DiagnosticGroups.td

--- include/clang/Basic/DiagnosticGroups.td (revision 188139)
+++ include/clang/Basic/DiagnosticGroups.td (working copy)
@@ -52,6 +52,7 @@
def ConfigMacros : DiagGroup<"config-macros">;
def : DiagGroup<"ctor-dtor-privacy">;
def GNUDesignator : DiagGroup<"gnu-designator">;
+def GNUTernaryElidingExpression : DiagGroup<"gnu-ternary-eliding-expression">;

def DeleteNonVirtualDtor : DiagGroup<"delete-non-virtual-dtor">;
def AbstractFinalClass : DiagGroup<"abstract-final-class">;
@@ -524,7 +525,8 @@

// A warning group for warnings about GCC extensions.
def GNU : DiagGroup<"gnu", [GNUDesignator, VLAExtension,
- ZeroLengthArray, GNUStaticFloatInit]>;
+ ZeroLengthArray, GNUStaticFloatInit,
+ GNUTernaryElidingExpression]>;
// A warning group for warnings about code that clang accepts but gcc doesn't.
def GccCompat : DiagGroup<"gcc-compat">;

Index: include/clang/Basic/DiagnosticParseKinds.td

--- include/clang/Basic/DiagnosticParseKinds.td (revision 188139)
+++ include/clang/Basic/DiagnosticParseKinds.td (working copy)
@@ -127,7 +127,7 @@
def ext_gnu_statement_expr : Extension<
   "use of GNU statement expression extension">, InGroup<GNU>;
def ext_gnu_conditional_expr : Extension<
- "use of GNU ?: expression extension, eliding middle term">, InGroup<GNU>;
+ "use of GNU ?: expression extension, eliding middle term">, InGroup<GNUTernaryElidingExpression>;
def ext_gnu_empty_initializer : Extension<
   "use of GNU empty initializer extension">, InGroup<GNU>;
def ext_gnu_array_range : Extension<"use of GNU array range extension">,

But I'm not familiar with clang's code, and so there may be more, or there may be tests that need doing, or documentation, or whatever else that goes along with this sort of thing.

I'm happy to put some effort in to the grunge work of doing this if someone more familiar with clang can set me on the right path as to what else needs to be done and what process is needed for submitting the patches, and presuming this is something that will be accepted into clang, and thus eventually work its way into Xcode.

You're on the right track - adding warning flags is a desired thing
(in fact, while we haven't gone to the effort of adding flags for
every warning, we have a check that ensures we don't add more
unflagged warnings (& that if we add flags to a warning we don't
regress this by removing those flags later)).

If you're interested in actually going through the whole process of
contributing a patch, you'll find the basics here:
http://llvm.org/docs/DeveloperPolicy.html (don't treat it all as
gospel - yeah, you don't need to be reading everything on the mailing
list to contribute one standalone patch).
http://llvm.org/docs/GettingStarted.html will get you through
compiling/building - once you've done that & made your change, if you
run the tests ("make check-clang" should do it) you'll probably notice
a test or two failing (the one that ensures we don't regress flags
being one I would expect - you'll need to tweak that list so that
these warnings no longer appear in it, this way we'll not regress this
by accident later on).

The only other thing would be to add a more specific test that
demonstrates this flag working - turning it on & off, etc. I'm not
sure exactly where those tests are, but you might look around for
something similar in tools/clang/test (perhaps if you look in the
version history for the last time the diagtool test was changed you'll
see the last new flag that was added and how other testing was
handled)

Hi Peter,

This change looks correct. There are two points to consider:
* choice of the option name. Does GCC have a similar option? Does
GCC have a name for this extension in the documentation?
* this change needs tests. You can model tests for this option based
on this example:

http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-static-const-float.cpp?revision=173841&view=markup

The new test file should go into the same directory as the example.

Dmitri

You're on the right track - adding warning flags is a desired thing
(in fact, while we haven't gone to the effort of adding flags for
every warning, we have a check that ensures we don't add more
unflagged warnings (& that if we add flags to a warning we don't
regress this by removing those flags later)).

Yes, I can see that, that is the llvm/tools/clang/test/Misc/warning-flags.c, but it doesn't apply in this specific case as the eliding middle term is covered under the -Wgnu group, it just doesn't have a separate flag. But hopefully it still falls under the same premise, that each feature should have a flag.

If you're interested in actually going through the whole process of
contributing a patch, you'll find the basics here:
http://llvm.org/docs/DeveloperPolicy.html (don't treat it all as
gospel - yeah, you don't need to be reading everything on the mailing
list to contribute one standalone patch).

I'm happy to provide a patch. I've got it compiling, got it testing, and I've added a test case for this issue as well.

So I can create a patch file, but I'm unsure of the next step. The Developer Policy says "Make your patch against the Subversion trunk", but for clang, it's go it's own trunk llvm/tools/clang, so I presume the patch should be relative to there. After I create the patch file, do I just post it to cfe-commits with a [PATHC] subject line and try to match whatever format other people have written, or is there some other process?

http://llvm.org/docs/GettingStarted.html will get you through
compiling/building - once you've done that & made your change, if you
run the tests ("make check-clang" should do it) you'll probably notice
a test or two failing (the one that ensures we don't regress flags
being one I would expect - you'll need to tweak that list so that
these warnings no longer appear in it, this way we'll not regress this
by accident later on).

Assuming I can get this patch through, I'm happy to spend a bit of effort knocking off some more of these cases, so I'll be happy to try to whittle that file down a bit.

The only other thing would be to add a more specific test that
demonstrates this flag working - turning it on & off, etc. I'm not
sure exactly where those tests are, but you might look around for
something similar in tools/clang/test (perhaps if you look in the
version history for the last time the diagtool test was changed you'll
see the last new flag that was added and how other testing was
handled)

Yep, no problem, I found a couple similar test cases and made a new one, which seems to work (failed until I compiled my fixes, and now passes).

Thanks for helping me get started on this,
   Peter.

This change looks correct. There are two points to consider:
* choice of the option name. Does GCC have a similar option? Does
GCC have a name for this extension in the documentation?

Very good question, to which I don't have a very good answer. There does not appear to be a matching GCC option.

GCC describes the issues as either "5.7 Conditionals with Omitted Operands", and mentions it as "the middle operand in a conditional expression may be omitted" or "omitting the middle term of a "?:" expression". The operator is either a "conditional expression" or a "ternary operator". And finally clang's warning is "use of GNU ?: expression extension, eliding middle term", making no mention of conditional, ternary or omitting.

So far I chose ternary-eliding-expression and gnu-ternary-eliding-expression, matching the behaviour of static-float-init and gnu-static-float-init, since despite it being originally a GNU extension, now it's a clang extension.

"conditional" is too vague. ternary-omitted-operand would be fine, but then I would change the warning in clang to "use of GNU ?: expression extension, omitting middle term" to match, and I wasn't sure I wanted to do that. I kind of prefer this one, but it's a slightly larger change, and I don't know the consequences of changing a warning message might have, though I suppose the warning will change slightly anyway with the introduction of the specific flag. I'm not sure what clang documentation might refer to this feature - is that something to worry about as well?

* this change needs tests. You can model tests for this option based on this example:

http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-static-const-float.cpp?revision=173841&view=markup

The new test file should go into the same directory as the example.

Yep, coincidently, that is *exactly* the test I based it on, although I decided it needed to be in the Sema directory instead because it's not a C++ issue.

Thanks,
   Peter.

> This change looks correct. There are two points to consider:
> * choice of the option name. Does GCC have a similar option? Does
> GCC have a name for this extension in the documentation?

Very good question, to which I don't have a very good answer. There does not appear to be a matching GCC option.

GCC describes the issues as either "5.7 Conditionals with Omitted Operands", and mentions it as "the middle operand in a conditional expression may be omitted" or "omitting the middle term of a "?:" expression". The operator is either a "conditional expression" or a "ternary operator". And finally clang's warning is "use of GNU ?: expression extension, eliding middle term", making no mention of conditional, ternary or omitting.

So far I chose ternary-eliding-expression and gnu-ternary-eliding-expression, matching the behaviour of static-float-init and gnu-static-float-init, since despite it being originally a GNU extension, now it's a clang extension.

"conditional" is too vague.

I disagree - this is the conditional operator. It also happens to be
the only ternary operator in the C++ language, but that's more luck
than good management (there's no fundamental reason there couldn't be
other ternary operators).

If we don't have any other flags for specific gnu extension then we've
no precedent for whether such flags should start with "gnu", but I
tend to think they should.

I'd imagine "-Wgnu-binary-conditional-operator" might make sense. (or
just -Wgnu-binary-conditional ? Not sure. People can bikeshed this as
they like - if you pick something, it goes in, people can speak up if
they find it particularly wrong)

ternary-omitted-operand would be fine, but then I would change the warning in clang to "use of GNU ?: expression extension, omitting middle term" to match, and I wasn't sure I wanted to do that. I kind of prefer this one, but it's a slightly larger change, and I don't know the consequences of changing a warning message might have, though I suppose the warning will change slightly anyway with the introduction of the specific flag.

Changing text is possible, if there's a clear improvement in
readability by doing so.

I'm not sure what clang documentation might refer to this feature - is that something to worry about as well?

Not generally - CLang doesn't have extensive documentation of its flags.