[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