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

[Dávid Bolvanský via cfe-commits]

Done


> Dňa 13. 8. 2020 o 7:29 užívateľ Arthur Eubanks via Phabricator 
>  napísal:
> 
> aeubanks added a comment.
> 
> It still seems to trigger on structs at head:
> 
> $ cat /tmp/a.cc
> struct A {
> 
>  const char *a;
>  const char *b;
>  const char *c;
> 
> };
> 
> static A a = {"",
> 
>  ""
>  "",
>  ""};
> 
> $ ./build/bin/clang++ -Wstring-concatenation -o /dev/null -c /tmp/a.cc
> C:/src/tmp/a.cc:10:15: warning: suspicious concatenation of string literals 
> in an array initialization; did you mean to separate the elements with a 
> comma? [-Wstring-concatenation]
> 
>  "",
>  ^
> 
> C:/src/tmp/a.cc:9:15: note: place parentheses around the string literal to 
> silence warning
> 
>  ""
>  ^
> 
> 1 warning generated.
> 
> 
> Repository:
>  rG LLVM Github Monorepo
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D85545/new/
> 
> https://reviews.llvm.org/D85545
> 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

It still seems to trigger on structs at head:

$ cat /tmp/a.cc
struct A {

  const char *a;
  const char *b;
  const char *c;

};

static A a = {"",

  ""
  "",
  ""};

$ ./build/bin/clang++ -Wstring-concatenation -o /dev/null -c /tmp/a.cc
C:/src/tmp/a.cc:10:15: warning: suspicious concatenation of string literals in 
an array initialization; did you mean to separate the elements with a comma? 
[-Wstring-concatenation]

  "",
  ^

C:/src/tmp/a.cc:9:15: note: place parentheses around the string literal to 
silence warning

  ""
  ^

1 warning generated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via cfe-commits]

Reworked ;) Now it should ignore structs properly.

ut 11. 8. 2020 o 22:03 Arthur O'Dwyer  napísal(a):
>
> Dávid: Please just disable it for initializers of structs. That seems to be 
> the common denominator in all the false positives I've observed on this 
> thread.
>
>
> On Tue, Aug 11, 2020 at 3:07 PM Dávid Bolvanský  
> wrote:
>>
>> Ok, I will bump that limit + 1.
>>
>> ut 11. 8. 2020 o 20:52 Arthur Eubanks via Phabricator
>>  napísal(a):
>> >
>> > aeubanks added a comment.
>> >
>> > In D85545#2211070 , @xbolva00 
>> > wrote:
>> >
>> > > I check if all elements of init list are strings, so this is not
>> > > exactly true "struct checker" (I have no such info in that part of
>> > > code..) but I consider it good enough. Your test case is based on real
>> > > code? Does not seem so - well sure, we could construct many examples
>> > > where warning fires uselessly but my focus is on real world false
>> > > positive cases :)
>> > >
>> > > ut 11. 8. 2020 o 19:55 Arthur Eubanks via Phabricator
>> > >  napísal(a):
>> > >
>> > >> aeubanks added a comment.
>> > >>
>> > >> Actually sorry, it does still seem like there are false positives on 
>> > >> structs. Reduced:
>> > >>
>> > >>   $ cat /tmp/a.cpp
>> > >>
>> > >>   struct A {
>> > >> const char* a;
>> > >> const char* b;
>> > >> const char* c;
>> > >>   };
>> > >>
>> > >>   static constexpr A foo2 = A{"",
>> > >>   ""
>> > >>   "",
>> > >>   ""};
>> > >>
>> > >>   $ ~/repos/llvm-project/build_cmake/bin/clang /tmp/a.cpp -o /dev/null 
>> > >> -c -Wstring-concatenation
>> > >>   /tmp/a.cpp:10:29: warning: suspicious concatenation of string 
>> > >> literals in an array initialization; did you mean to separate the 
>> > >> elements with a comma? [-Wstring-concatenation]
>> > >>   "",
>> > >>   ^
>> > >>   /tmp/a.cpp:9:29: note: place parentheses around the string literal to 
>> > >> silence warning
>> > >>   ""
>> > >>   ^
>> > >>   1 warning generated.
>> > >>
>> > >> Repository:
>> > >>
>> > >>   rG LLVM Github Monorepo
>> > >>
>> > >> CHANGES SINCE LAST ACTION
>> > >>
>> > >>   https://reviews.llvm.org/D85545/new/
>> > >>
>> > >> https://reviews.llvm.org/D85545
>> >
>> > Yup it's reduced from real world code, same link as before: 
>> > https://source.chromium.org/chromium/chromium/src/+/master:third_party/dawn/src/dawn_native/Toggles.cpp;drc=80f927d763211ea8e6a6377f86282809c86dc107;l=32.
>> > Clearly if there are 3 strings in the struct and the initializer has three 
>> > strings, then the warning shouldn't apply.
>> >
>> >
>> > Repository:
>> >   rG LLVM Github Monorepo
>> >
>> > CHANGES SINCE LAST ACTION
>> >   https://reviews.llvm.org/D85545/new/
>> >
>> > https://reviews.llvm.org/D85545
>> >
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Arthur O'Dwyer via cfe-commits]

Dávid: Please just disable it for initializers of structs. That seems to be
the common denominator in all the false positives I've observed on this
thread.


On Tue, Aug 11, 2020 at 3:07 PM Dávid Bolvanský 
wrote:

> Ok, I will bump that limit + 1.
>
> ut 11. 8. 2020 o 20:52 Arthur Eubanks via Phabricator
>  napísal(a):
> >
> > aeubanks added a comment.
> >
> > In D85545#2211070 , @xbolva00
> wrote:
> >
> > > I check if all elements of init list are strings, so this is not
> > > exactly true "struct checker" (I have no such info in that part of
> > > code..) but I consider it good enough. Your test case is based on real
> > > code? Does not seem so - well sure, we could construct many examples
> > > where warning fires uselessly but my focus is on real world false
> > > positive cases :)
> > >
> > > ut 11. 8. 2020 o 19:55 Arthur Eubanks via Phabricator
> > >  napísal(a):
> > >
> > >> aeubanks added a comment.
> > >>
> > >> Actually sorry, it does still seem like there are false positives on
> structs. Reduced:
> > >>
> > >>   $ cat /tmp/a.cpp
> > >>
> > >>   struct A {
> > >> const char* a;
> > >> const char* b;
> > >> const char* c;
> > >>   };
> > >>
> > >>   static constexpr A foo2 = A{"",
> > >>   ""
> > >>   "",
> > >>   ""};
> > >>
> > >>   $ ~/repos/llvm-project/build_cmake/bin/clang /tmp/a.cpp -o
> /dev/null -c -Wstring-concatenation
> > >>   /tmp/a.cpp:10:29: warning: suspicious concatenation of string
> literals in an array initialization; did you mean to separate the elements
> with a comma? [-Wstring-concatenation]
> > >>   "",
> > >>   ^
> > >>   /tmp/a.cpp:9:29: note: place parentheses around the string literal
> to silence warning
> > >>   ""
> > >>   ^
> > >>   1 warning generated.
> > >>
> > >> Repository:
> > >>
> > >>   rG LLVM Github Monorepo
> > >>
> > >> CHANGES SINCE LAST ACTION
> > >>
> > >>   https://reviews.llvm.org/D85545/new/
> > >>
> > >> https://reviews.llvm.org/D85545
> >
> > Yup it's reduced from real world code, same link as before:
> https://source.chromium.org/chromium/chromium/src/+/master:third_party/dawn/src/dawn_native/Toggles.cpp;drc=80f927d763211ea8e6a6377f86282809c86dc107;l=32
> .
> > Clearly if there are 3 strings in the struct and the initializer has
> three strings, then the warning shouldn't apply.
> >
> >
> > Repository:
> >   rG LLVM Github Monorepo
> >
> > CHANGES SINCE LAST ACTION
> >   https://reviews.llvm.org/D85545/new/
> >
> > https://reviews.llvm.org/D85545
> >
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via cfe-commits]

Ok, I will bump that limit + 1.

ut 11. 8. 2020 o 20:52 Arthur Eubanks via Phabricator
 napísal(a):
>
> aeubanks added a comment.
>
> In D85545#2211070 , @xbolva00 wrote:
>
> > I check if all elements of init list are strings, so this is not
> > exactly true "struct checker" (I have no such info in that part of
> > code..) but I consider it good enough. Your test case is based on real
> > code? Does not seem so - well sure, we could construct many examples
> > where warning fires uselessly but my focus is on real world false
> > positive cases :)
> >
> > ut 11. 8. 2020 o 19:55 Arthur Eubanks via Phabricator
> >  napísal(a):
> >
> >> aeubanks added a comment.
> >>
> >> Actually sorry, it does still seem like there are false positives on 
> >> structs. Reduced:
> >>
> >>   $ cat /tmp/a.cpp
> >>
> >>   struct A {
> >> const char* a;
> >> const char* b;
> >> const char* c;
> >>   };
> >>
> >>   static constexpr A foo2 = A{"",
> >>   ""
> >>   "",
> >>   ""};
> >>
> >>   $ ~/repos/llvm-project/build_cmake/bin/clang /tmp/a.cpp -o /dev/null -c 
> >> -Wstring-concatenation
> >>   /tmp/a.cpp:10:29: warning: suspicious concatenation of string literals 
> >> in an array initialization; did you mean to separate the elements with a 
> >> comma? [-Wstring-concatenation]
> >>   "",
> >>   ^
> >>   /tmp/a.cpp:9:29: note: place parentheses around the string literal to 
> >> silence warning
> >>   ""
> >>   ^
> >>   1 warning generated.
> >>
> >> Repository:
> >>
> >>   rG LLVM Github Monorepo
> >>
> >> CHANGES SINCE LAST ACTION
> >>
> >>   https://reviews.llvm.org/D85545/new/
> >>
> >> https://reviews.llvm.org/D85545
>
> Yup it's reduced from real world code, same link as before: 
> https://source.chromium.org/chromium/chromium/src/+/master:third_party/dawn/src/dawn_native/Toggles.cpp;drc=80f927d763211ea8e6a6377f86282809c86dc107;l=32.
> Clearly if there are 3 strings in the struct and the initializer has three 
> strings, then the warning shouldn't apply.
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D85545/new/
>
> https://reviews.llvm.org/D85545
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D85545#2211070 , @xbolva00 wrote:

> I check if all elements of init list are strings, so this is not
> exactly true "struct checker" (I have no such info in that part of
> code..) but I consider it good enough. Your test case is based on real
> code? Does not seem so - well sure, we could construct many examples
> where warning fires uselessly but my focus is on real world false
> positive cases :)
>
> ut 11. 8. 2020 o 19:55 Arthur Eubanks via Phabricator
>  napísal(a):
>
>> aeubanks added a comment.
>>
>> Actually sorry, it does still seem like there are false positives on 
>> structs. Reduced:
>>
>>   $ cat /tmp/a.cpp
>>   
>>   struct A {
>> const char* a;
>> const char* b;
>> const char* c;
>>   };
>>   
>>   static constexpr A foo2 = A{"",
>>   ""
>>   "",
>>   ""};
>>   
>>   $ ~/repos/llvm-project/build_cmake/bin/clang /tmp/a.cpp -o /dev/null -c 
>> -Wstring-concatenation
>>   /tmp/a.cpp:10:29: warning: suspicious concatenation of string literals in 
>> an array initialization; did you mean to separate the elements with a comma? 
>> [-Wstring-concatenation]
>>   "",
>>   ^
>>   /tmp/a.cpp:9:29: note: place parentheses around the string literal to 
>> silence warning
>>   ""
>>   ^
>>   1 warning generated.
>>
>> Repository:
>>
>>   rG LLVM Github Monorepo
>>
>> CHANGES SINCE LAST ACTION
>>
>>   https://reviews.llvm.org/D85545/new/
>>
>> https://reviews.llvm.org/D85545

Yup it's reduced from real world code, same link as before: 
https://source.chromium.org/chromium/chromium/src/+/master:third_party/dawn/src/dawn_native/Toggles.cpp;drc=80f927d763211ea8e6a6377f86282809c86dc107;l=32.
Clearly if there are 3 strings in the struct and the initializer has three 
strings, then the warning shouldn't apply.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via cfe-commits]

I check if all elements of init list are strings, so this is not
exactly true "struct checker" (I have no such info in that part of
code..) but I consider it good enough. Your test case is based on real
code? Does not seem so - well sure, we could construct many examples
where warning fires uselessly but my focus is on real world false
positive cases :)

ut 11. 8. 2020 o 19:55 Arthur Eubanks via Phabricator
 napísal(a):
>
> aeubanks added a comment.
>
> Actually sorry, it does still seem like there are false positives on structs. 
> Reduced:
>
>   $ cat /tmp/a.cpp
>
>   struct A {
> const char* a;
> const char* b;
> const char* c;
>   };
>
>   static constexpr A foo2 = A{"",
>   ""
>   "",
>   ""};
>
>   $ ~/repos/llvm-project/build_cmake/bin/clang /tmp/a.cpp -o /dev/null -c 
> -Wstring-concatenation
>   /tmp/a.cpp:10:29: warning: suspicious concatenation of string literals in 
> an array initialization; did you mean to separate the elements with a comma? 
> [-Wstring-concatenation]
>   "",
>   ^
>   /tmp/a.cpp:9:29: note: place parentheses around the string literal to 
> silence warning
>   ""
>   ^
>   1 warning generated.
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D85545/new/
>
> https://reviews.llvm.org/D85545
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

Actually sorry, it does still seem like there are false positives on structs. 
Reduced:

  $ cat /tmp/a.cpp
  
  struct A {
const char* a;
const char* b;
const char* c;
  };
  
  static constexpr A foo2 = A{"",
  ""
  "",
  ""};
  
  $ ~/repos/llvm-project/build_cmake/bin/clang /tmp/a.cpp -o /dev/null -c 
-Wstring-concatenation
  /tmp/a.cpp:10:29: warning: suspicious concatenation of string literals in an 
array initialization; did you mean to separate the elements with a comma? 
[-Wstring-concatenation]
  "",
  ^
  /tmp/a.cpp:9:29: note: place parentheses around the string literal to silence 
warning
  ""
  ^
  1 warning generated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Thanks for feedback:)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

After not warning on structs, this did find a real missing comma in the 
Chromium codebase, so thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via cfe-commits]

Pushed fix. It should not warn for structs or long texts.

ut 11. 8. 2020 o 0:34 Arthur Eubanks via Phabricator
 napísal(a):
>
> aeubanks added a comment.
>
> In D85545#2208266 , @arthur.j.odwyer 
> wrote:
>
> > 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
>
> +1 to ignoring structs. See https://crbug.com/1114873 and 
> https://source.chromium.org/chromium/chromium/src/+/master:third_party/dawn/src/dawn_native/Toggles.cpp;drc=80f927d763211ea8e6a6377f86282809c86dc107;l=32.
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D85545/new/
>
> https://reviews.llvm.org/D85545
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D85545#2208266 , @arthur.j.odwyer 
wrote:

> 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

+1 to ignoring structs. See https://crbug.com/1114873 and 
https://source.chromium.org/chromium/chromium/src/+/master:third_party/dawn/src/dawn_native/Toggles.cpp;drc=80f927d763211ea8e6a6377f86282809c86dc107;l=32.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via cfe-commits]

For your godbolt example, you hit the heuristic to not warn, if
literals count is <= 2. (motivated by many false positives; and I
didnt see a true positive case, so..)

>> you could emit the warning only if exactly one comma was missing.

This could work.

ut 11. 8. 2020 o 0:04 Arthur O'Dwyer  napísal(a):
>
> 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ý  
> 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ý  
>> 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  
>> > 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ý 
>> > >  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
>> > >>  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
>> > >> >
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [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ý 
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ý 
> 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 
> 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.bolvan...@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
> > >>  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
> > >> >
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via cfe-commits]

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ý  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  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ý  
> > 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
> >>  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
> >> >
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via cfe-commits]

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  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ý  
> 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
>>  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
>> >
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Arthur O'Dwyer via cfe-commits]

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ý 
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
>  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
> >
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via cfe-commits]

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
 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
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[dmajor via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via cfe-commits]

Logs look quite old.

http://lab.llvm.org:8011/builders/ppc64le-lld-multistage-test/builds/11291/steps/build-stage2-unified-tree/logs/stdio
are newest logs? Maybe I can tune the warning more to avoid false
positives.

po 10. 8. 2020 o 21:46 Kamau Bridgeman via Phabricator
 napísal(a):
>
> kamaub added a comment.
>
> Hello, sorry but can you please revert this commit and recommit it when you 
> have a fix or work around that doesn't break our bots:
> It breaks 
> http://lab.llvm.org:8011/builders/ppc64le-lld-multistage-test/builds/11228 
> which builds with `-Werror`
> Please also note that it introduced 103 warnings in 
> clang-ppc64le-linux-multistage/builds/13042 
> 
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D85545/new/
>
> https://reviews.llvm.org/D85545
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kamau Bridgeman via Phabricator via cfe-commits]

kamaub added a comment.

Hello, sorry but can you please revert this commit and recommit it when you 
have a fix or work around that doesn't break our bots:
It breaks 
http://lab.llvm.org:8011/builders/ppc64le-lld-multistage-test/builds/11228 
which builds with `-Werror`
Please also note that it introduced 103 warnings in 
clang-ppc64le-linux-multistage/builds/13042 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

And we caught one more bug in LLVM repo (in libcxx)

  const char* TestCaseSetOne[] = {"", "s", "bac",
  "bacasf"
  "lkajseravea",
  "adsfkajdsfjkas;lnc441324513,34535r34525234",
  "b*c",
  "ba?sf"
  "lka*ea",
  "adsf*kas;lnc441[0-9]1r34525234"};

https://github.com/llvm/llvm-project/blob/daacf57032450079b44b8a7f9b976700d3bc38f8/libcxx/test/libcxx/fuzzing/fuzzer_test.h#L20


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGdc096a66cb51: [Diagnostics] Diagnose missing comma in string 
array initialization (authored by xbolva00).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/string-concat.c

Index: clang/test/Sema/string-concat.c
===
--- /dev/null
+++ clang/test/Sema/string-concat.c
@@ -0,0 +1,102 @@
+
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s
+
+const char *missing_comma[] = {
+"basic_filebuf",
+"basic_ios",
+"future",
+"optional",
+"packaged_task" // expected-note{{place parentheses around the string literal to silence warning}}
+"promise",  // expected-warning{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}} 
+"shared_future"
+};
+
+#ifndef __cplusplus
+typedef __WCHAR_TYPE__ wchar_t;
+#endif
+
+const wchar_t *missing_comma_wchar[] = {
+L"basic_filebuf",
+L"packaged_task" // expected-note{{place parentheses around the string literal to silence warning}}
+L"promise"  // expected-warning{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
+};
+
+const char *missing_comma_u8[] = {
+u8"basic_filebuf",
+u8"packaged_task" // expected-note{{place parentheses around the string literal to silence warning}}
+u8"promise"  // expected-warning{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
+};
+
+const char *missing_two_commas[] = {"basic_filebuf",
+   "basic_ios" // expected-note{{place parentheses around the string literal to silence warning}}
+   "future"// expected-warning{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
+   "optional",
+   "packaged_task"};
+
+const char *missing_comma_same_line[] = {"basic_filebuf", "basic_ios",
+   "future" "optional", // expected-note{{place parentheses around the string literal to silence warning}}
+   "packaged_task", "promise"}; // expected-warning@-1{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
+
+const char *missing_comma_different_lines[] = {"basic_filebuf", "basic_ios" // expected-note{{place parentheses around the string literal to silence warning}}
+   "future", "optional",// expected-warning{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
+   "packaged_task", "promise"};
+
+const char *missing_comma_same_line_all_literals[] = {"basic_filebuf", "future" "optional", "packaged_task"}; // expected-note{{place parentheses around the string literal to silence warning}}
+   // expected-warning@-1{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
+
+char missing_comma_inner[][4] = {
+"a",
+"b" // expected-note{{place parentheses around the string literal to silence warning}}
+"c" // expected-warning{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
+};
+
+
+#define ONE(x) x
+#define TWO "foo"
+const char *macro_test[] = { ONE("foo") "bar", 
+ TWO "bar", 
+ "foo" TWO // expected-note{{place parentheses around the string literal to silence warning}}
+   };  // expected-warning@-1{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
+
+// Do not warn for macros.
+
+#define BASIC_IOS "basic_ios"
+#define FUTURE "future"
+const char *macro_test2[] = {"basic_filebuf", BASIC_IOS
+FUTURE, "optional",
+   "packaged_task", "promise"};
+
+#define FOO(xx) xx "_normal", \
+xx "_movable",
+
+const char *macro_test3[] = {"basic_filebuf",
+   "basic_ios",
+   FOO("future")
+   "optional",
+   "packaged_task"};
+
+#define BAR(name) #name "_normal"
+
+const char *macro_test4[] = {"basic_filebuf",
+   "basic_ios",
+   BAR(fut

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Thank you all for code review


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 284136.
xbolva00 added a comment.

Fixed nit


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/string-concat.c

Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -6907,6 +6907,21 @@
 << DIE->getSourceRange();
   Diag(InitArgList[I]->getBeginLoc(), diag::note_designated_init_mixed)
 << InitArgList[I]->getSourceRange();
+} else if (const auto *SL = dyn_cast(InitArgList[I])) {
+  unsigned NumConcat = SL->getNumConcatenated();
+  // Diagnose missing comma in string array initialization.
+  // Do not warn when all the elements in the initializer are concatenated together.
+  // Do not warn for macros too.
+  if (NumConcat > 1 && E > 1 && !SL->getBeginLoc().isMacroID()) {
+SmallVector Hints;
+for (unsigned i = 0; i < NumConcat - 1; ++i)
+  Hints.push_back(FixItHint::CreateInsertion(
+  PP.getLocForEndOfToken(SL->getStrTokenLoc(i)), ", "));
+
+Diag(SL->getStrTokenLoc(1), diag::warn_concatenated_literal_array_init)
+<< Hints;
+Diag(SL->getBeginLoc(), diag::note_concatenated_string_literal_silence);
+  }
 }
   }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2999,6 +2999,10 @@
 def warn_objc_string_literal_comparison : Warning<
   "direct comparison of a string literal has undefined behavior">,
   InGroup;
+def warn_concatenated_literal_array_init : Warning<
+  "suspicious concatenation of string literals in an array initialization; "
+  "did you mean to separate the elements with a comma?">,
+  InGroup>;
 def warn_concatenated_nsarray_literal : Warning<
   "concatenated NSString literal for an NSArray expression - "
   "possibly missing a comma">,
@@ -6139,6 +6143,9 @@
 def note_evaluate_comparison_first :Note<
   "place parentheses around comparison expression to evaluate it first">;
 
+def note_concatenated_string_literal_silence :Note<
+  "place parentheses around the string literal to silence warning">;
+
 def warn_addition_in_bitshift : Warning<
   "operator '%0' has lower precedence than '%1'; "
   "'%1' will be evaluated first">, InGroup;
Index: clang/test/Sema/string-concat.c
===
--- /dev/null
+++ clang/test/Sema/string-concat.c
@@ -0,0 +1,102 @@
+
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s
+
+const char *missing_comma[] = {
+"basic_filebuf",
+"basic_ios",
+"future",
+"optional",
+"packaged_task" // expected-note{{place parentheses around the string literal to silence warning}}
+"promise",  // expected-warning{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}} 
+"shared_future"
+};
+
+#ifndef __cplusplus
+typedef __WCHAR_TYPE__ wchar_t;
+#endif
+
+const wchar_t *missing_comma_wchar[] = {
+L"basic_filebuf",
+L"packaged_task" // expected-note{{place parentheses around the string literal to silence warning}}
+L"promise"  // expected-warning{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
+};
+
+const char *missing_comma_u8[] = {
+u8"basic_filebuf",
+u8"packaged_task" // expected-note{{place parentheses around the string literal to silence warning}}
+u8"promise"  // expected-warning{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
+};
+
+const char *missing_two_commas[] = {"basic_filebuf",
+   "basic_ios" // expected-note{{place parentheses around the string literal to silence warning}}
+   "future"// expected-warning{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
+   "optional",
+   "packaged_task"};
+
+const char *missing_comma_same_line[] = {"basic_filebuf", "basic_ios",
+   "future" "optional", // expected-note{{place parentheses around the string literal to silence warning}}
+   "packaged_task", "promise"}; // expected-warning@-1{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
+
+const char *missing_comma_different_lines[] = {"basic_filebuf", "basic_ios" // expected-note{{place parentheses aroun

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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

LGTM; nothing further from me!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from a nit, but you should give other reviewers a chance to comment 
if they have additional feedback.




Comment at: clang/lib/Sema/SemaExpr.cpp:6911
+} else if (const auto *SL = dyn_cast(InitArgList[I])) {
+  unsigned numConcat = SL->getNumConcatenated();
+  // Diagnose missing comma in string array initialization.

`numConcat` -> `NumConcat`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 284128.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/string-concat.c

Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -6907,6 +6907,21 @@
 << DIE->getSourceRange();
   Diag(InitArgList[I]->getBeginLoc(), diag::note_designated_init_mixed)
 << InitArgList[I]->getSourceRange();
+} else if (const auto *SL = dyn_cast(InitArgList[I])) {
+  unsigned numConcat = SL->getNumConcatenated();
+  // Diagnose missing comma in string array initialization.
+  // Do not warn when all the elements in the initializer are concatenated together.
+  // Do not warn for macros too.
+  if (numConcat > 1 && E > 1 && !SL->getBeginLoc().isMacroID()) {
+SmallVector Hints;
+for (unsigned i = 0; i < numConcat - 1; ++i)
+  Hints.push_back(FixItHint::CreateInsertion(
+  PP.getLocForEndOfToken(SL->getStrTokenLoc(i)), ", "));
+
+Diag(SL->getStrTokenLoc(1), diag::warn_concatenated_literal_array_init)
+<< Hints;
+Diag(SL->getBeginLoc(), diag::note_concatenated_string_literal_silence);
+  }
 }
   }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2999,6 +2999,10 @@
 def warn_objc_string_literal_comparison : Warning<
   "direct comparison of a string literal has undefined behavior">,
   InGroup;
+def warn_concatenated_literal_array_init : Warning<
+  "suspicious concatenation of string literals in an array initialization; "
+  "did you mean to separate the elements with a comma?">,
+  InGroup>;
 def warn_concatenated_nsarray_literal : Warning<
   "concatenated NSString literal for an NSArray expression - "
   "possibly missing a comma">,
@@ -6139,6 +6143,9 @@
 def note_evaluate_comparison_first :Note<
   "place parentheses around comparison expression to evaluate it first">;
 
+def note_concatenated_string_literal_silence :Note<
+  "place parentheses around the string literal to silence warning">;
+
 def warn_addition_in_bitshift : Warning<
   "operator '%0' has lower precedence than '%1'; "
   "'%1' will be evaluated first">, InGroup;
Index: clang/test/Sema/string-concat.c
===
--- /dev/null
+++ clang/test/Sema/string-concat.c
@@ -0,0 +1,102 @@
+
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s
+
+const char *missing_comma[] = {
+"basic_filebuf",
+"basic_ios",
+"future",
+"optional",
+"packaged_task" // expected-note{{place parentheses around the string literal to silence warning}}
+"promise",  // expected-warning{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}} 
+"shared_future"
+};
+
+#ifndef __cplusplus
+typedef __WCHAR_TYPE__ wchar_t;
+#endif
+
+const wchar_t *missing_comma_wchar[] = {
+L"basic_filebuf",
+L"packaged_task" // expected-note{{place parentheses around the string literal to silence warning}}
+L"promise"  // expected-warning{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
+};
+
+const char *missing_comma_u8[] = {
+u8"basic_filebuf",
+u8"packaged_task" // expected-note{{place parentheses around the string literal to silence warning}}
+u8"promise"  // expected-warning{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
+};
+
+const char *missing_two_commas[] = {"basic_filebuf",
+   "basic_ios" // expected-note{{place parentheses around the string literal to silence warning}}
+   "future"// expected-warning{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
+   "optional",
+   "packaged_task"};
+
+const char *missing_comma_same_line[] = {"basic_filebuf", "basic_ios",
+   "future" "optional", // expected-note{{place parentheses around the string literal to silence warning}}
+   "packaged_task", "promise"}; // expected-warning@-1{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
+
+const char *missing_comma_different_lines[] = {"basic_filebuf", "basic_ios" // expected-note{{place parentheses around the string literal to silence warnin

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 284127.
xbolva00 added a comment.

Addressed review notes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/string-concat.c

Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -6907,6 +6907,21 @@
 << DIE->getSourceRange();
   Diag(InitArgList[I]->getBeginLoc(), diag::note_designated_init_mixed)
 << InitArgList[I]->getSourceRange();
+} else if (const auto *SL = dyn_cast(InitArgList[I])) {
+  unsigned numConcat = SL->getNumConcatenated();
+  // Diagnose missing comma in string array initialization.
+  // Do not warn when all the elements in the initializer are concatenated together.
+  // Do not warn for macros too.
+  if (numConcat > 1 && E > 1 && !SL->getBeginLoc().isMacroID()) {
+SmallVector Hints;
+for (unsigned i = 0; i < numConcat - 1; ++i)
+  Hints.push_back(FixItHint::CreateInsertion(
+  PP.getLocForEndOfToken(SL->getStrTokenLoc(i)), ", "));
+
+Diag(SL->getStrTokenLoc(1), diag::warn_concatenated_literal_array_init)
+<< Hints;
+Diag(SL->getBeginLoc(), diag::note_concatenated_string_literal_silence);
+  }
 }
   }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2999,6 +2999,10 @@
 def warn_objc_string_literal_comparison : Warning<
   "direct comparison of a string literal has undefined behavior">,
   InGroup;
+def warn_concatenated_literal_array_init : Warning<
+  "suspicious concatenation of string literals in an array initialization; "
+  "did you mean to separate the elements with a comma?">,
+  InGroup>;
 def warn_concatenated_nsarray_literal : Warning<
   "concatenated NSString literal for an NSArray expression - "
   "possibly missing a comma">,
@@ -6139,6 +6143,9 @@
 def note_evaluate_comparison_first :Note<
   "place parentheses around comparison expression to evaluate it first">;
 
+def note_concatenated_string_literal_silence :Note<
+  "place parentheses around the string literal to silence warning">;
+
 def warn_addition_in_bitshift : Warning<
   "operator '%0' has lower precedence than '%1'; "
   "'%1' will be evaluated first">, InGroup;
Index: clang/test/Sema/string-concat.c
===
--- /dev/null
+++ clang/test/Sema/string-concat.c
@@ -0,0 +1,103 @@
+
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s
+
+const char *missing_comma[] = {
+"basic_filebuf",
+"basic_ios",
+"future",
+"optional",
+"packaged_task" // expected-note{{place parentheses around the string literal to silence warning}}
+"promise",  // expected-warning{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}} 
+"shared_future"
+};
+
+#ifndef __cplusplus
+typedef __WCHAR_TYPE__ wchar_t;
+#endif
+
+const wchar_t *missing_comma_wchar[] = {
+L"basic_filebuf",
+L"packaged_task" // expected-note{{place parentheses around the string literal to silence warning}}
+L"promise"  // expected-warning{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
+};
+
+const char *missing_comma_u8[] = {
+u8"basic_filebuf",
+u8"packaged_task" // expected-note{{place parentheses around the string literal to silence warning}}
+u8"promise"  // expected-warning{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
+};
+
+const char *missing_two_commas[] = {"basic_filebuf",
+   "basic_ios" // expected-note{{place parentheses around the string literal to silence warning}}
+   "future"// expected-warning{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
+   "optional",
+   "packaged_task"};
+
+const char *missing_comma_same_line[] = {"basic_filebuf", "basic_ios",
+   "future" "optional", // expected-note{{place parentheses around the string literal to silence warning}}
+   "packaged_task", "promise"}; // expected-warning@-1{{suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?}}
+
+const char *missing_comma_different_lines[] = {"basic_filebuf", "basic_ios" // expected-note{{place par

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3003
+def warn_concatenated_literal_array_init : Warning<
+  "concatenated literal in a string array initialization - "
+  "possibly missing a comma">,

How about: `suspicious concatenation of string literals in an array 
initialization; did you mean to separate the elements with a comma?`



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6147
+def note_concatenated_string_literal_silence :Note<
+  "place parentheses around string literal to silence warning">;
+

around string literal -> around the string literal



Comment at: clang/lib/Sema/SemaExpr.cpp:6916
+for (unsigned i = 0; i < numConcat; ++i)
+  if (SL->getStrTokenLoc(i).isMacroID()) {
+hasMacro = true;

xbolva00 wrote:
> Quuxplusone wrote:
> > I wonder if perhaps the warning should trigger only if the concatenated 
> > pieces appear one-per-line, i.e., whitespace-sensitive.
> > 
> > const char a[] = {
> > "a"
> > "b"
> > };
> > const char b[] = {
> > "b" "c"
> > };
> > 
> > It's at least arguable that `a` is a bug and `b` is intentional, based 
> > purely on how the whitespace appears. OTOH, whitespace is not preserved by 
> > clang-format, and it would suck for clang-formatting the code to cause the 
> > appearance (or disappearance) of diagnostics.
> Hard to tell, no strong opinion.
> 
> We can always decrease the "power" of warning based on external feedback.
I feel like it could go either way and really depends more on the number of 
initializers and other heuristic patterns than the whitespace. e.g., `{"a" "b", 
"c" "d"}` seems more likely to be correct than `{"a", "b" "c", "d"}` and it's 
sort of impossible to tell with `{"a" "b"}` what is intended, while `{"a", "b", 
"c", "d", "e" "f", "g", "h"}` is quite likely a bug.

I think the only scenario we should NOT warn on initially is when all the 
elements in the initializer are concatenated together because it seems 
plausible that would be done for ease of formatting. e.g., `{"a" "b"}`, `{"a" 
"b" "c" }`, etc.



Comment at: clang/test/Sema/string-concat.c:86
+};
\ No newline at end of file


Please add the newline to the end of the file.

Also, I'd like to see tests with other string literal types, like `L` or `u8` 
just to demonstrate that this isn't specific to narrow string literals.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:6916
+for (unsigned i = 0; i < numConcat; ++i)
+  if (SL->getStrTokenLoc(i).isMacroID()) {
+hasMacro = true;

Quuxplusone wrote:
> I wonder if perhaps the warning should trigger only if the concatenated 
> pieces appear one-per-line, i.e., whitespace-sensitive.
> 
> const char a[] = {
> "a"
> "b"
> };
> const char b[] = {
> "b" "c"
> };
> 
> It's at least arguable that `a` is a bug and `b` is intentional, based purely 
> on how the whitespace appears. OTOH, whitespace is not preserved by 
> clang-format, and it would suck for clang-formatting the code to cause the 
> appearance (or disappearance) of diagnostics.
Hard to tell, no strong opinion.

We can always decrease the "power" of warning based on external feedback.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:6921
+if (!hasMacro)
+  Diag(SL->getBeginLoc(), diag::warn_concatenated_literal_array_init);
+  }

riccibruno wrote:
> Should this point to the location of the suspected missing comma?
Yeah, +1. And we should emit fixit too.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 284043.
xbolva00 marked 2 inline comments as done.
xbolva00 added a comment.

New testcases.
Emit note to tell user how to supress warning - extra parentheses.
Emit fixit with comma.
Addressed review notes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/string-concat.c

Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -6907,6 +6907,19 @@
 << DIE->getSourceRange();
   Diag(InitArgList[I]->getBeginLoc(), diag::note_designated_init_mixed)
 << InitArgList[I]->getSourceRange();
+} else if (const auto *SL = dyn_cast(InitArgList[I])) {
+  unsigned numConcat = SL->getNumConcatenated();
+  // Diagnose missing comma in string array initialization.
+  if (numConcat > 1 && !SL->getBeginLoc().isMacroID()) {
+SmallVector Hints;
+for (unsigned i = 0; i < numConcat - 1; ++i)
+  Hints.push_back(FixItHint::CreateInsertion(
+  PP.getLocForEndOfToken(SL->getStrTokenLoc(i)), ", "));
+
+Diag(SL->getStrTokenLoc(1), diag::warn_concatenated_literal_array_init)
+<< Hints;
+Diag(SL->getBeginLoc(), diag::note_concatenated_string_literal_silence);
+  }
 }
   }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2999,6 +2999,10 @@
 def warn_objc_string_literal_comparison : Warning<
   "direct comparison of a string literal has undefined behavior">,
   InGroup;
+def warn_concatenated_literal_array_init : Warning<
+  "concatenated literal in a string array initialization - "
+  "possibly missing a comma">,
+  InGroup>;
 def warn_concatenated_nsarray_literal : Warning<
   "concatenated NSString literal for an NSArray expression - "
   "possibly missing a comma">,
@@ -6139,6 +6143,9 @@
 def note_evaluate_comparison_first :Note<
   "place parentheses around comparison expression to evaluate it first">;
 
+def note_concatenated_string_literal_silence :Note<
+  "place parentheses around string literal to silence warning">;
+
 def warn_addition_in_bitshift : Warning<
   "operator '%0' has lower precedence than '%1'; "
   "'%1' will be evaluated first">, InGroup;
Index: clang/test/Sema/string-concat.c
===
--- /dev/null
+++ clang/test/Sema/string-concat.c
@@ -2,40 +2,84 @@
 // RUN: %clang_cc1 -x c -fsyntax-only -verify %s
 // RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s
 
-const char *test1[] = {
+const char *missing_comma[] = {
 "basic_filebuf",
 "basic_ios",
 "future",
 "optional",
-"packaged_task" // expected-warning{{concatenated literal in a string array initialization - possibly missing a comma}}
-"promise",
-"shared_future",
-"shared_lock",
-"shared_ptr",
-"thread",
-"unique_ptr",
-"unique_lock",
-"weak_ptr",
+"packaged_task" // expected-note{{place parentheses around string literal to silence warning}}
+"promise",  // expected-warning{{concatenated literal in a string array initialization - possibly missing a comma}}
+"promise",  
+"shared_future"
 };
 
-const char *test2[] = {"basic_filebuf",
-   "basic_ios" // expected-warning{{concatenated literal in a string array initialization - possibly missing a comma}}
-   "future"
+const char *missing_two_commas[] = {"basic_filebuf",
+   "basic_ios" // expected-note{{place parentheses around string literal to silence warning}}
+   "future"// expected-warning{{concatenated literal in a string array initialization - possibly missing a comma}}
"optional",
"packaged_task"};
 
-const char *test3[] = {"basic_filebuf", "basic_ios",
-   "future" "optional", // expected-warning{{concatenated literal in a string array initialization - possibly missing a comma}}
-   "packaged_task", "promise"};
+const char *missing_comma_same_line[] = {"basic_filebuf", "basic_ios",
+   "future" "optional", // expected-note{{place parentheses around string literal to silence warning}}
+   "packaged_task", "promise"}; // expected-warning@-1{{concatenated literal in a string array initialization - possibly missing a comma}}
 
-const char *test4[] = {"basic_filebuf", "basic_ios" // expected-warning{{concatenated literal in a string array initialization - possibly missing a comma}}
-   "future", "optional",
+const char *missing_comma_different_lines[]

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



Comment at: clang/test/Sema/string-concat.c:55
+   "optional",
+   "packaged_task"};

Quuxplusone wrote:
> > What if the user did actually want to concatenate the strings
> 
> > Is there a way to suppress this diagnostic
> 
> Sounds like if someone wanted to suppress the diagnostic, it would suffice 
> for them to add a macro invocation:
> 
> #define SUPPRESS(x) x
> const char *test9[] = { SUPPRESS("foo" "bar"), "baz" };
> 
> Please add this test case. Please also add one for concatenation //with the 
> result of// a macro, as in
> 
> #define ONE(x) x
> #define TWO "foo"
> const char *test10[] = { ONE("foo") "bar", TWO "bar", "foo" TWO };
> 
> I would expect and hope that the diagnostic would catch all three of these 
> cases.
> 
> It would also be nice to check a case like
> 
> char test11[][4] = {
> "a",
> "b"
> "c"
> };
> 
> where the string literal is being used to initialize an array not a pointer.
```
#define SUPPRESS(x) x
const char *test9[] = { SUPPRESS("foo" "bar"), "baz" };
```

Added.


```
#define ONE(x) x
#define TWO "foo"
const char *test10[] = { ONE("foo") "bar", TWO "bar", "foo" TWO };
```

Only "foo" TWO is diagnosted. Other two may lead to false positives - already 
tested with linux kernel.



Comment at: clang/test/Sema/string-concat.c:55
+   "optional",
+   "packaged_task"};

xbolva00 wrote:
> Quuxplusone wrote:
> > > What if the user did actually want to concatenate the strings
> > 
> > > Is there a way to suppress this diagnostic
> > 
> > Sounds like if someone wanted to suppress the diagnostic, it would suffice 
> > for them to add a macro invocation:
> > 
> > #define SUPPRESS(x) x
> > const char *test9[] = { SUPPRESS("foo" "bar"), "baz" };
> > 
> > Please add this test case. Please also add one for concatenation //with the 
> > result of// a macro, as in
> > 
> > #define ONE(x) x
> > #define TWO "foo"
> > const char *test10[] = { ONE("foo") "bar", TWO "bar", "foo" TWO };
> > 
> > I would expect and hope that the diagnostic would catch all three of these 
> > cases.
> > 
> > It would also be nice to check a case like
> > 
> > char test11[][4] = {
> > "a",
> > "b"
> > "c"
> > };
> > 
> > where the string literal is being used to initialize an array not a pointer.
> ```
> #define SUPPRESS(x) x
> const char *test9[] = { SUPPRESS("foo" "bar"), "baz" };
> ```
> 
> Added.
> 
> 
> ```
> #define ONE(x) x
> #define TWO "foo"
> const char *test10[] = { ONE("foo") "bar", TWO "bar", "foo" TWO };
> ```
> 
> Only "foo" TWO is diagnosted. Other two may lead to false positives - already 
> tested with linux kernel.
```
char test11[][4] = {
"a",
"b"
"c"
};
```

This case is diagnosed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:6916
+for (unsigned i = 0; i < numConcat; ++i)
+  if (SL->getStrTokenLoc(i).isMacroID()) {
+hasMacro = true;

I wonder if perhaps the warning should trigger only if the concatenated pieces 
appear one-per-line, i.e., whitespace-sensitive.

const char a[] = {
"a"
"b"
};
const char b[] = {
"b" "c"
};

It's at least arguable that `a` is a bug and `b` is intentional, based purely 
on how the whitespace appears. OTOH, whitespace is not preserved by 
clang-format, and it would suck for clang-formatting the code to cause the 
appearance (or disappearance) of diagnostics.



Comment at: clang/test/Sema/string-concat.c:55
+   "optional",
+   "packaged_task"};

> What if the user did actually want to concatenate the strings

> Is there a way to suppress this diagnostic

Sounds like if someone wanted to suppress the diagnostic, it would suffice for 
them to add a macro invocation:

#define SUPPRESS(x) x
const char *test9[] = { SUPPRESS("foo" "bar"), "baz" };

Please add this test case. Please also add one for concatenation //with the 
result of// a macro, as in

#define ONE(x) x
#define TWO "foo"
const char *test10[] = { ONE("foo") "bar", TWO "bar", "foo" TWO };

I would expect and hope that the diagnostic would catch all three of these 
cases.

It would also be nice to check a case like

char test11[][4] = {
"a",
"b"
"c"
};

where the string literal is being used to initialize an array not a pointer.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D85545#2203661 , @xbolva00 wrote:

> Parentheses could work here

Yay great! Let's add such test?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

Is there a way to suppress this diagnostic if someone wants to legitimately 
initialize an element of the array with a long string by relying on string 
literal concatenation?




Comment at: clang/lib/Sema/SemaExpr.cpp:6910
 << InitArgList[I]->getSourceRange();
+} else if (StringLiteral *SL = dyn_cast(InitArgList[I])) {
+  unsigned numConcat = SL->getNumConcatenated();

`const auto *`



Comment at: clang/lib/Sema/SemaExpr.cpp:6921
+if (!hasMacro)
+  Diag(SL->getBeginLoc(), diag::warn_concatenated_literal_array_init);
+  }

Should this point to the location of the suspected missing comma?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Do not warn for macros (found false positives when compiling linux kernel)

In D85545#2203660 , @NoQ wrote:

> What if the user did actually want to concatenate the strings? Eg., one of 
> the strings in the list is long and clang-format suggests breaking it up for 
> the 80-column limit which causes the new warning to appear. How would the 
> user suppress the warning in this case?

Parentheses could work here:

const char *test1[] = {

  "basic_filebuf",
  "basic_ios",
  "future",
  "optional",
  ("packaged_task"
  "promise"),
  "shared_future"

};


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

What if the user did actually want to concatenate the strings? Eg., one of the 
strings in the list is long and clang-format suggests breaking it up for the 
80-column limit which causes the new warning to appear. How would the user 
suppress the warning in this case?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 284002.
xbolva00 added a comment.

Do not warn for macros - found false positives when compiling linux kernel.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/string-concat.c


Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -6907,6 +6907,19 @@
 << DIE->getSourceRange();
   Diag(InitArgList[I]->getBeginLoc(), diag::note_designated_init_mixed)
 << InitArgList[I]->getSourceRange();
+} else if (StringLiteral *SL = dyn_cast(InitArgList[I])) {
+  unsigned numConcat = SL->getNumConcatenated();
+  // Diagnose missing comma in string array initialization.
+  if (numConcat > 1) {
+bool hasMacro = false;
+for (unsigned i = 0; i < numConcat; ++i)
+  if (SL->getStrTokenLoc(i).isMacroID()) {
+hasMacro = true;
+break;
+  }
+if (!hasMacro)
+  Diag(SL->getBeginLoc(), diag::warn_concatenated_literal_array_init);
+  }
 }
   }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2999,6 +2999,10 @@
 def warn_objc_string_literal_comparison : Warning<
   "direct comparison of a string literal has undefined behavior">,
   InGroup;
+def warn_concatenated_literal_array_init : Warning<
+  "concatenated literal in a string array initialization - "
+  "possibly missing a comma">,
+  InGroup>;
 def warn_concatenated_nsarray_literal : Warning<
   "concatenated NSString literal for an NSArray expression - "
   "possibly missing a comma">,
Index: clang/test/Sema/string-concat.c
===
--- /dev/null
+++ clang/test/Sema/string-concat.c
@@ -9,13 +9,7 @@
 "optional",
 "packaged_task" // expected-warning{{concatenated literal in a string 
array initialization - possibly missing a comma}}
 "promise",
-"shared_future",
-"shared_lock",
-"shared_ptr",
-"thread",
-"unique_ptr",
-"unique_lock",
-"weak_ptr",
+"shared_future"
 };
 
 const char *test2[] = {"basic_filebuf",
@@ -32,10 +26,30 @@
"future", "optional",
"packaged_task", "promise"};
 
+const char *test5[] = {"basic_filebuf", "future" "optional", "packaged_task"}; 
// expected-warning{{concatenated literal in a string array initialization - 
possibly missing a comma}}
+
+
+// Do not warn for macros.
+
 #define BASIC_IOS "basic_ios"
 #define FUTURE "future"
-const char *test5[] = {"basic_filebuf", BASIC_IOS // 
expected-warning{{concatenated literal in a string array initialization - 
possibly missing a comma}}
+const char *test6[] = {"basic_filebuf", BASIC_IOS
 FUTURE, "optional",
"packaged_task", "promise"};
 
-const char *test6[] = {"basic_filebuf", "future" "optional", "packaged_task"}; 
// expected-warning{{concatenated literal in a string array initialization - 
possibly missing a comma}}
+#define FOO(xx) xx "_normal", \
+xx "_movable",
+
+const char *test7[] = {"basic_filebuf",
+   "basic_ios"
+   FOO("future")
+   "optional",
+   "packaged_task"};
+
+#define BAR(name) #name "_normal"
+
+const char *test8[] = {"basic_filebuf",
+   "basic_ios"
+   BAR(future),
+   "optional",
+   "packaged_task"};


Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -6907,6 +6907,19 @@
 << DIE->getSourceRange();
   Diag(InitArgList[I]->getBeginLoc(), diag::note_designated_init_mixed)
 << InitArgList[I]->getSourceRange();
+} else if (StringLiteral *SL = dyn_cast(InitArgList[I])) {
+  unsigned numConcat = SL->getNumConcatenated();
+  // Diagnose missing comma in string array initialization.
+  if (numConcat > 1) {
+bool hasMacro = false;
+for (unsigned i = 0; i < numConcat; ++i)
+  if (SL->getStrTokenLoc(i).isMacroID()) {
+hasMacro = true;
+break;
+  }
+if (!hasMacro)
+  Diag(SL->getBeginLoc(), diag::warn_concatenated_literal_array_init);
+  }
 }
   }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2999,6 +2999,10 

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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 created this revision.
xbolva00 added a reviewer: aaron.ballman.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
xbolva00 requested review of this revision.

Motivation (from PR37674):

const char *ss[] = {

  "foo", "bar",
  "baz", "qux"  // <-- Missing comma!
  "abc", "xyz"
  };

This kind of bug was recently also found in LLVM codebase (see PR47038).

Solves PR47038, PR37674


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D85545

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/string-concat.c


Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -6907,6 +6907,11 @@
 << DIE->getSourceRange();
   Diag(InitArgList[I]->getBeginLoc(), diag::note_designated_init_mixed)
 << InitArgList[I]->getSourceRange();
+} else if (StringLiteral *SL = dyn_cast(InitArgList[I])) {
+  unsigned numConcat = SL->getNumConcatenated();
+  // Diagnose missing comma in string array initialization.
+  if (numConcat > 1)
+Diag(SL->getBeginLoc(), diag::warn_concatenated_literal_array_init);
 }
   }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2999,6 +2999,10 @@
 def warn_objc_string_literal_comparison : Warning<
   "direct comparison of a string literal has undefined behavior">,
   InGroup;
+def warn_concatenated_literal_array_init : Warning<
+  "concatenated literal in a string array initialization - "
+  "possibly missing a comma">,
+  InGroup>;
 def warn_concatenated_nsarray_literal : Warning<
   "concatenated NSString literal for an NSArray expression - "
   "possibly missing a comma">,
Index: clang/test/Sema/string-concat.c
===
--- /dev/null
+++ clang/test/Sema/string-concat.c
@@ -0,0 +1,41 @@
+
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s
+
+const char *test1[] = {
+"basic_filebuf",
+"basic_ios",
+"future",
+"optional",
+"packaged_task" // expected-warning{{concatenated literal in a string 
array initialization - possibly missing a comma}}
+"promise",
+"shared_future",
+"shared_lock",
+"shared_ptr",
+"thread",
+"unique_ptr",
+"unique_lock",
+"weak_ptr",
+};
+
+const char *test2[] = {"basic_filebuf",
+   "basic_ios" // expected-warning{{concatenated literal 
in a string array initialization - possibly missing a comma}}
+   "future"
+   "optional",
+   "packaged_task"};
+
+const char *test3[] = {"basic_filebuf", "basic_ios",
+   "future" "optional", // expected-warning{{concatenated 
literal in a string array initialization - possibly missing a comma}}
+   "packaged_task", "promise"};
+
+const char *test4[] = {"basic_filebuf", "basic_ios" // 
expected-warning{{concatenated literal in a string array initialization - 
possibly missing a comma}}
+   "future", "optional",
+   "packaged_task", "promise"};
+
+#define BASIC_IOS "basic_ios"
+#define FUTURE "future"
+const char *test5[] = {"basic_filebuf", BASIC_IOS // 
expected-warning{{concatenated literal in a string array initialization - 
possibly missing a comma}}
+FUTURE, "optional",
+   "packaged_task", "promise"};
+
+const char *test6[] = {"basic_filebuf", "future" "optional", "packaged_task"}; 
// expected-warning{{concatenated literal in a string array initialization - 
possibly missing a comma}}


Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -6907,6 +6907,11 @@
 << DIE->getSourceRange();
   Diag(InitArgList[I]->getBeginLoc(), diag::note_designated_init_mixed)
 << InitArgList[I]->getSourceRange();
+} else if (StringLiteral *SL = dyn_cast(InitArgList[I])) {
+  unsigned numConcat = SL->getNumConcatenated();
+  // Diagnose missing comma in string array initialization.
+  if (numConcat > 1)
+Diag(SL->getBeginLoc(), diag::warn_concatenated_literal_array_init);
 }
   }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2999,6 +2999,10 @@
 def warn_objc_string_literal_comparison : Warning<
   "direct comparison of a string literal has undefined behavior">,
   InGroup;
+def warn_concatenated_literal_array_init