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

Reply via email to