Page MenuHomePhabricator

[Diagnostics] Diagnose missing comma in string array initialization
ClosedPublic

Authored by xbolva00 on Aug 7 2020, 12:17 PM.

Details

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

Diff Detail

Event Timeline

xbolva00 created this revision.Aug 7 2020, 12:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2020, 12:17 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
xbolva00 requested review of this revision.Aug 7 2020, 12:17 PM
xbolva00 edited the summary of this revision. (Show Details)
xbolva00 updated this revision to Diff 284002.Aug 7 2020, 12:53 PM

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

NoQ added a subscriber: NoQ.Aug 7 2020, 12:59 PM

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 added a comment.EditedAug 7 2020, 1:07 PM

Parentheses could work here:

const char *test1[] = {

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

};

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 added a comment.Aug 7 2020, 1:28 PM

Parentheses could work here

Yay great! Let's add such test?

Quuxplusone added inline comments.
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 added inline comments.Aug 7 2020, 2:03 PM
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 updated this revision to Diff 284043.Aug 7 2020, 2:07 PM
xbolva00 marked 2 inline comments as done.

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

xbolva00 added inline comments.Aug 7 2020, 2:15 PM
clang/lib/Sema/SemaExpr.cpp
6920

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

xbolva00 added inline comments.Aug 7 2020, 2:29 PM
clang/lib/Sema/SemaExpr.cpp
6915

Hard to tell, no strong opinion.

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

RKSimon added a subscriber: RKSimon.Aug 8 2020, 1:28 AM
aaron.ballman added inline comments.Aug 8 2020, 5:04 AM
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 updated this revision to Diff 284127.Aug 8 2020, 6:03 AM

Addressed review notes.

xbolva00 marked 4 inline comments as done.Aug 8 2020, 6:03 AM
xbolva00 updated this revision to Diff 284128.Aug 8 2020, 6:07 AM
aaron.ballman accepted this revision.Aug 8 2020, 6:13 AM

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

This revision is now accepted and ready to land.Aug 8 2020, 6:13 AM

LGTM; nothing further from me!

xbolva00 updated this revision to Diff 284136.Aug 8 2020, 10:18 AM

Fixed nit

Thank you all for code review

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 added a subscriber: kamaub.Aug 10 2020, 12:46 PM

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

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
<reviews@reviews.llvm.org> 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 added a subscriber: dmajor.Aug 10 2020, 2:21 PM

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?

I moved it to -Wextra due to false positives.

Should there be some exception for line length

Yeah, but sure how to define the threshold or so. :/

po 10. 8. 2020 o 23:21 dmajor via Phabricator
<reviews@reviews.llvm.org> napísal(a):

dmajor added a comment.

In the Firefox repo this warning is firing on a number of strings that were broken up by clang-format (or humans) for line length, for example https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/security/certverifier/ExtendedValidation.cpp#176-178 or https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/xpcom/tests/gtest/TestEscape.cpp#103-104 or https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/js/src/jsapi-tests/testXDR.cpp#115.

Do you consider these to be false positives in your view? Should there be some exception for line length, perhaps?

Repository:

rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION

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

https://reviews.llvm.org/D85545

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

For your cases, we currently do not warn. If possible, fetch the
latest Clang trunk and re-evaluate on Firefox.

po 10. 8. 2020 o 23:46 Arthur O'Dwyer <arthur.j.odwyer@gmail.com> napísal(a):

It looks to me as if all of the false-positives so far have been not arrays but structs.

struct X { int a; const char *b; int c; };
X x = { 41, "forty" "two", 43 }; // false-positive here

The distinguishing feature here is that if you did insert a comma as suggested by the compiler, then the result would no longer type-check.
X x = { 41, "forty", "two", 43 }; // this is ill-formed because "two" is not a valid initializer for int c

Dávid, can you use this in some way?
IMHO it would be appropriate to just turn the warning off if the entity being initialized is a struct — leave the warning enabled only for initializers of arrays.

my $.02,
–Arthur

Something like this:

const char *Sources[] = {
  "// \\tparam aaa Bbb\n",
  "// \\tparam\n"
  "//     aaa Bbb\n",
  "// \\tparam \n"
  "//     aaa Bbb\n",
  "// \\tparam aaa\n"
  "// Bbb\n"
};

annoys me :/ Any idea/heuristic how to avoid warning here?

po 10. 8. 2020 o 23:48 Dávid Bolvanský <david.bolvansky@gmail.com> napísal(a):

For your cases, we currently do not warn. If possible, fetch the
latest Clang trunk and re-evaluate on Firefox.

po 10. 8. 2020 o 23:46 Arthur O'Dwyer <arthur.j.odwyer@gmail.com> napísal(a):

It looks to me as if all of the false-positives so far have been not arrays but structs.

struct X { int a; const char *b; int c; };
X x = { 41, "forty" "two", 43 }; // false-positive here

The distinguishing feature here is that if you did insert a comma as suggested by the compiler, then the result would no longer type-check.
X x = { 41, "forty", "two", 43 }; // this is ill-formed because "two" is not a valid initializer for int c

Dávid, can you use this in some way?
IMHO it would be appropriate to just turn the warning off if the entity being initialized is a struct — leave the warning enabled only for initializers of arrays.

my $.02,
–Arthur

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

I moved it to -Wextra due to false positives.

Should there be some exception for line length

Yeah, but sure how to define the threshold or so. :/

po 10. 8. 2020 o 23:21 dmajor via Phabricator
<reviews@reviews.llvm.org> napísal(a):

dmajor added a comment.

In the Firefox repo this warning is firing on a number of strings that were broken up by clang-format (or humans) for line length, for example https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/security/certverifier/ExtendedValidation.cpp#176-178 or https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/xpcom/tests/gtest/TestEscape.cpp#103-104 or https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/js/src/jsapi-tests/testXDR.cpp#115.

Do you consider these to be false positives in your view? Should there be some exception for line length, perhaps?

Repository:

rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION

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

https://reviews.llvm.org/D85545

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

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 <arthur.j.odwyer@gmail.com> 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

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.

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

ut 11. 8. 2020 o 0:34 Arthur Eubanks via Phabricator
<reviews@reviews.llvm.org> 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

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

Thanks for feedback:)

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.

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
<reviews@reviews.llvm.org> 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

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
<reviews@reviews.llvm.org> 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.

Ok, I will bump that limit + 1.

ut 11. 8. 2020 o 20:52 Arthur Eubanks via Phabricator
<reviews@reviews.llvm.org> 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
<reviews@reviews.llvm.org> 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

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.

Reworked ;) Now it should ignore structs properly.

ut 11. 8. 2020 o 22:03 Arthur O'Dwyer <arthur.j.odwyer@gmail.com> 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 added a comment.EditedAug 12 2020, 10:29 PM

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.

Done

Dňa 13. 8. 2020 o 7:29 užívateľ Arthur Eubanks via Phabricator <reviews@reviews.llvm.org> 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