[cfe-commits] [Patch] -Wformat-non-standard: warn about positional arguments (pr12017)

[Ted Kremenek]

Hi Hans,

I think I only have a minor nit:

+  "the '%0' %1 is not supported by ISO C">, InGroup<FormatNonStandard>, DefaultIgnore;

It seems weird to start a diagnostic with "the".  Why include it?  Dropping it changes:

  the 'q' length modifier is not supported by ISO C

to

  'q' length modifier is not supported by ISO C

The second seems just fine for a warning.

We can also probably be a bit more succinct with this diagnostic:

  "using the length modifier '%0' with the conversion specifier '%1' is not supported by ISO C"

can instead be:

 "using length modifier '%0' with conversion specifier '%1' is not supported by ISO C"

The second wording is more direct, and a bit more clinical, but the more verbose version doesn't add much value and just makes the warning longer.

On Mar 7, 2012, at 10:00 AM, Hans Wennborg <hans at chromium.org> wrote:

> On Thu, Mar 1, 2012 at 19:00, Ted Kremenek <kremenek at apple.com> wrote:
>> On Feb 28, 2012, at 3:03 AM, Hans Wennborg <hans at chromium.org> wrote:
>>> On Mon, Feb 27, 2012 at 20:00, Ted Kremenek <kremenek at apple.com> wrote:
>>>> For me the goal of the warning is to warn about non-portable code, not annoy
>>>> people.  Format specifiers and format string extensions covered by POSIX are
>>>> by definition portable on POSIX-compliant systems.  So I raise the question
>>>> of whether or not we should warn about these at all?
>>> 
>>> I agree that a good warning about non-portable code, that could be
>>> turned on by default or as part of -Wall, would be the ideal.
>>> 
>>> However, we're not there yet. In the meantime, I think having a
>>> warning under -pedantic that warns about non-ISO C format strings
>>> makes sense. I agree that it would be extremely annoying to warn about
>>> POSIX extensions by default, but under -pedantic I think users would
>>> expect to get warnings about these, just as with GCC.
>> 
>> Ok, I'm fine with this approach (putting under -pedantic), but should we put it under a separate warning group (that is activated by -Wformat-non-standard).  That way people could turn off these warnings if all the care about is POSIX compatibility by still keep the rest of the portability warnings.
>> 
>>> 
>>> Maybe the wording of the warnings and the name of the flag could be
>>> changed to make this intention more clear. I agree that just saying
>>> "non-standard" is a bit vague in the light of some of these features
>>> actually being standardized under POSIX.
>> 
>> I think improving the wording would help quite a bit.  What is "standard" is also a moving target, so saying what the "standard" is might help a great deal (e.g., C99).  It also may just add confusion.  Another way is to have -Wformat-posix-extensions, which is activated by -Wformat-non-standard (as I suggested above), and just have a parenthetical note in the warning that says "POSIX extension".
> 
> 
> Attaching a new patch that re-names the warning flag and rewords the
> warnings a little bit. Hopefully this makes it much more clear.
> 
> Thanks,
> Hans
> <positional-arguments-warning2.diff>