[cfe-dev] Explicit warning diagnostic flags for groups
Peter N Lewis
peter at stairways.com.au
Tue Aug 13 06:34:38 PDT 2013
On 13/08/2013, at 13:48 , David Blaikie <dblaikie at gmail.com> wrote:
> 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.
On 13/08/2013, at 13:48 , David Blaikie <dblaikie at gmail.com> wrote:
> On Mon, Aug 12, 2013 at 10:30 PM, Peter N Lewis <peter at stairways.com.au> wrote:
>> 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)
--
Keyboard Maestro 6.1 now out - set web checkboxes & radio buttons, exit from loops, and more.
Keyboard Maestro <http://www.keyboardmaestro.com/> Macros for your Mac
<http://www.stairways.com/> <http://download.keyboardmaestro.com/>
More information about the cfe-dev
mailing list