[Diagnostics] Diagnose missing comma in string array initialization

[xbolva00]

Summary

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 PR47030).

Solves PR47038, PR37674

[xbolva00]

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

[NoQ]

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?

[xbolva00]

Parentheses could work here:

const char *test1[] = {

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

};

[riccibruno]

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?

clang/lib/Sema/SemaExpr.cpp

6909

const auto *

6920

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

[NoQ]

In D85545#2203661, @xbolva00 wrote:

Parentheses could work here

Yay great! Let's add such test?

[• Quuxplusone]

clang/lib/Sema/SemaExpr.cpp

6915

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.

clang/test/Sema/string-concat.c

55

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.

[xbolva00]

clang/test/Sema/string-concat.c

55

#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.

55

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

This case is diagnosed.

[xbolva00]

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

[xbolva00]

clang/lib/Sema/SemaExpr.cpp

6920

Yeah, +1. And we should emit fixit too.

[xbolva00]

clang/lib/Sema/SemaExpr.cpp

6915

Hard to tell, no strong opinion.

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

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

3006

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

6150

around string literal -> around the string literal

clang/lib/Sema/SemaExpr.cpp

6915

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.

clang/test/Sema/string-concat.c

86

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.

[xbolva00]

Addressed review notes.

[aaron.ballman]

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

clang/lib/Sema/SemaExpr.cpp

6910

numConcat -> NumConcat

[• Quuxplusone]

LGTM; nothing further from me!

[xbolva00]

Fixed nit

[xbolva00]

Thank you all for code review

[xbolva00]

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

[kamaub]

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

[xbolva00]

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
<[e-mail]> 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 http://lab.llvm.org:8011/builders/clang-ppc64le-linux-multistage/builds/13042/steps/ninja%20check%202/logs/warnings%20%28106%29

Repository:

rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION

https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545

[dmajor]

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?

[xbolva00]

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
<[e-mail]> 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

[• arthur.j.odwyer]

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

[xbolva00]

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 <[e-mail]> 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

[xbolva00]

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ý <[e-mail]> 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 <[e-mail]> 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ý <[e-mail]> 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
<[e-mail]> 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

[• arthur.j.odwyer]

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

[xbolva00]

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 <[e-mail]> 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

[aeubanks]

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.

[xbolva00]

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

ut 11. 8. 2020 o 0:34 Arthur Eubanks via Phabricator
<[e-mail]> napísal(a):

aeubanks added a comment.

In D85545#2208266 https://reviews.llvm.org/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

[aeubanks]

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

[xbolva00]

Thanks for feedback:)

[aeubanks]

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.

[xbolva00]

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
<[e-mail]> 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

[aeubanks]

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
<[e-mail]> 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.

[xbolva00]

Ok, I will bump that limit + 1.

ut 11. 8. 2020 o 20:52 Arthur Eubanks via Phabricator
<[e-mail]> napísal(a):

aeubanks added a comment.

In D85545#2211070 https://reviews.llvm.org/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
<[e-mail]> 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

[• arthur.j.odwyer]

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.

[xbolva00]

Reworked ;) Now it should ignore structs properly.

ut 11. 8. 2020 o 22:03 Arthur O'Dwyer <[e-mail]> 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.

[aeubanks]

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.

[xbolva00]

Done

Dňa 13. 8. 2020 o 7:29 užívateľ Arthur Eubanks via Phabricator <[e-mail]> 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

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_objc_literal_comparison : Warning<
   "direct comparison of %select{an array literal|a dictionary literal|"
   "a numeric literal|a boxed expression|}0 has undefined behavior">,
   InGroup<ObjCLiteralComparison>;
 def err_missing_atsign_prefix : Error<
   "%select{string|numeric}0 literal must be prefixed by '@'">;
 def warn_objc_string_literal_comparison : Warning<
   "direct comparison of a string literal has undefined behavior">,
   InGroup<ObjCStringComparison>;
+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<DiagGroup<"string-concatenation">>;
 def warn_concatenated_nsarray_literal : Warning<
   "concatenated NSString literal for an NSArray expression - "
   "possibly missing a comma">,
   InGroup<ObjCStringConcatenation>;
 def note_objc_literal_comparison_isequal : Note<
   "use 'isEqual:' instead">;
 def warn_objc_collection_literal_element : Warning<
   "object of type %0 is not compatible with "

 
 def warn_overloaded_shift_in_comparison :Warning<
   "overloaded operator %select{>>|<<}0 has higher precedence than "
   "comparison operator">,
   InGroup<OverloadedShiftOpParentheses>;
 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<ShiftOpParentheses>;
 
 def warn_self_assignment_builtin : Warning<
   "explicitly assigning value of variable of type %0 to itself">,
   InGroup<SelfAssignment>, DefaultIgnore;
 def warn_self_assignment_overloaded : Warning<

clang/lib/Sema/SemaExpr.cpp

for (unsigned I = 0, E = InitArgList.size(); I != E; ++I) {
     } else if (getLangOpts().CPlusPlus && !DiagnosedMixedDesignator &&
                isa<DesignatedInitExpr>(InitArgList[0])) {
       DiagnosedMixedDesignator = true;
       auto *DIE = cast<DesignatedInitExpr>(InitArgList[0]);
       Diag(DIE->getBeginLoc(), diag::ext_designated_init_mixed)
         << DIE->getSourceRange();
       Diag(InitArgList[I]->getBeginLoc(), diag::note_designated_init_mixed)
         << InitArgList[I]->getSourceRange();
+    } else if (const auto *SL = dyn_cast<StringLiteral>(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<FixItHint, 1> 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);
+      }
     }
   }
 
   if (FirstDesignator.isValid()) {
     // Only diagnose designated initiaization as a C++20 extension if we didn't
     // already diagnose use of (non-C++20) C99 designator syntax.
     if (getLangOpts().CPlusPlus && !DiagnosedArrayDesignator &&
         !DiagnosedNestedDesignator && !DiagnosedMixedDesignator) {

clang/test/Sema/string-concat.c

+
+// 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(future),
+                       "optional",
+                       "packaged_task"};
+
+#define SUPPRESS(x) x
+const char *macro_test5[] = { SUPPRESS("foo" "bar"), "baz" };
+
+// Do not warn when all the elements in the initializer are concatenated together.
+const char *all_elems_in_init_concatenated[] = {"a" "b" "c"};
+
+// Warning can be supressed also by extra parentheses.
+const char *extra_parens_to_suppress_warning[] = {
+    "basic_filebuf",
+    "basic_ios",
+    "future",
+    "optional",
+    ("packaged_task"
+    "promise"),
+    "shared_future"
+};