[PATCH] D85545: [Diagnostics] Diagnose missing comma in string array initialization

[Arthur O'Dwyer via cfe-commits]

To decrease the number of false-positives, you could emit the warning only
if *exactly one* comma was missing.

    const char *likely_a_bug[] = { "a", "b", "c" "d", "e", "f", "g", "h",
"i" };
    const char *likely_not_a_bug[] = { "a", "b" "c", "d" "e", "f" "g" };
    const char *oops_still_a_bug[] = { "a", "b", "c" "d", "e", "f" "g",
"h", "i" };

However, as `oops_still_a_bug` shows, that tactic would also decrease the
number of true positives, and it would confuse the end-user, for whom
predictability is key.

I still think it would be appropriate to *stop issuing the warning for
structs*, though.
Here's my struct example from below in Godbolt: https://godbolt.org/z/6jjv6a
Speaking of predictability, I don't understand why `struct Y` avoids the
warning whereas `struct X` hits it.
After removing the warning for structs, neither `X` nor `Y` should hit it,
and that should fix pretty much all the Firefox hits as I understand them.

–Arthur


On Mon, Aug 10, 2020 at 5:51 PM Dávid Bolvanský <david.bolvansky at gmail.com>
wrote:

> Something like this:
>   const char *Sources[] = {
>     "// \\tparam aaa Bbb\n",
>     "// \\tparam\n"
>     "//     aaa Bbb\n",
>     "// \\tparam \n"
>     "//     aaa Bbb\n",
>     "// \\tparam aaa\n"
>     "// Bbb\n"
>   };
>
> annoys me :/ Any idea/heuristic how to avoid warning here?
>
> po 10. 8. 2020 o 23:48 Dávid Bolvanský <david.bolvansky at gmail.com>
> napísal(a):
> >
> > For your cases, we currently do not warn. If possible, fetch the
> > latest Clang trunk and re-evaluate on Firefox.
> >
> > po 10. 8. 2020 o 23:46 Arthur O'Dwyer <arthur.j.odwyer at gmail.com>
> napísal(a):
> > >
> > > It looks to me as if all of the false-positives so far have been not
> arrays but structs.
> > >
> > > struct X { int a; const char *b; int c; };
> > > X x = { 41, "forty" "two", 43 };  // false-positive here
> > >
> > > The distinguishing feature here is that if you did insert a comma as
> suggested by the compiler, then the result would no longer type-check.
> > > X x = { 41, "forty", "two", 43 };  // this is ill-formed because "two"
> is not a valid initializer for `int c`
> > >
> > > Dávid, can you use this in some way?
> > > IMHO it would be appropriate to just turn the warning off if the
> entity being initialized is a struct — leave the warning enabled only for
> initializers of arrays.
> > >
> > > my $.02,
> > > –Arthur
> > >
> > >
> > > On Mon, Aug 10, 2020 at 5:38 PM Dávid Bolvanský <
> david.bolvansky at gmail.com> wrote:
> > >>
> > >> I moved it to -Wextra due to false positives.
> > >>
> > >> > Should there be some exception for line length
> > >>
> > >> Yeah, but sure how to define the threshold or so. :/
> > >>
> > >> po 10. 8. 2020 o 23:21 dmajor via Phabricator
> > >> <reviews at reviews.llvm.org> napísal(a):
> > >> >
> > >> > dmajor added a comment.
> > >> >
> > >> > In the Firefox repo this warning is firing on a number of strings
> that were broken up by clang-format (or humans) for line length, for
> example
> https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/security/certverifier/ExtendedValidation.cpp#176-178
> or
> https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/xpcom/tests/gtest/TestEscape.cpp#103-104
> or
> https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/js/src/jsapi-tests/testXDR.cpp#115
> .
> > >> >
> > >> > Do you consider these to be false positives in your view? Should
> there be some exception for line length, perhaps?
> > >> >
> > >> >
> > >> > Repository:
> > >> >   rG LLVM Github Monorepo
> > >> >
> > >> > CHANGES SINCE LAST ACTION
> > >> >   https://reviews.llvm.org/D85545/new/
> > >> >
> > >> > https://reviews.llvm.org/D85545
> > >> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200810/17adf6aa/attachment-0001.html>