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

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 = {"", > > ""

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

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:

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

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

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

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 Phab

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

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

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

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 i

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

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 useless

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

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{"", ""

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

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://li

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

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 _

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

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

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

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",

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

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í

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

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[

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

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ávi

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

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; in

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

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 r

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

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

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

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

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

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 ad

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

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 in

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

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",

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

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

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

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/li

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

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-c

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

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/mailma

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

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 +

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

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/SemaExp

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

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

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

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: `s

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

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 s

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

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 mi

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

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

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

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

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

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

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

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 __

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

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]->g

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

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-

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

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? CHANGE

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

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/DiagnosticSem

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

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! "ab