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

[Arthur O'Dwyer via Phabricator via cfe-commits]

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


================
Comment at: clang/test/Sema/string-concat.c:55
+                       "optional",
+                       "packaged_task"};
----------------
> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85545/new/

https://reviews.llvm.org/D85545