[PATCH] D156565: Diagnose use of VLAs in C++ by default

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Overall looks good, just a few nits from looking things over.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:143
+def ext_vla_cxx : ExtWarn<
+  "variable length arrays are a Clang extension">, InGroup<VLAExtension>;
+def ext_vla_cxx_in_gnu_mode : Extension<ext_vla_cxx.Summary>,
----------------
Clarify text: "variable length arrays in C++ are a Clang extension" and same below.

I also think we should be (consistently) using the term "variable-length array" with a dash instead of "variable length array", but if you change that, it should be in a follow-on which changes it in all the messages.


================
Comment at: clang/lib/Sema/SemaType.cpp:2617
+  } else if (getLangOpts().CPlusPlus) {
+    if (getLangOpts().CPlusPlus11 && IsStaticAssertLike(ArraySize, Context))
+      VLADiag = getLangOpts().GNUMode
----------------
Not sure whether to actually care, since C++98 is so old now, but, having `-Wno-vla-extension-static-assert` work in C++98 mode seems like it'd be quite useful, exactly because the usage cannot trivially be replaced by a static_assert. So it's a bit unfortunate that we don't distinguish it there.

Perhaps we should emit the same diagnostics in both 98/11, but with slightly different text in 98?


================
Comment at: clang/test/CXX/dcl.dcl/dcl.spec/dcl.typedef/p2-0x.cpp:39
+  using T = int[n]; // expected-error {{variable length array declaration not allowed at file scope}} \
+                       expected-warning {{variable length arrays are a Clang extension}} \
+                       expected-note {{read of non-const variable 'n' is not allowed in a constant expression}}
----------------
That we get a warning _and_ an error for this using statement now is non-ideal. I see that we are doing this in two different places, though...first in CheckType for the VLA, then diagnosing if it's used at file scope in CheckDecl. So maybe not worth fixing.



================
Comment at: clang/test/SemaCXX/cxx1z-noexcept-function-type.cpp:124
+  typedef int arr[strcmp("bar", "foo") + 4 * strncmp("foo", "bar", 4)]; // expected-warning {{variable length array folded to constant array as an extension}} \
+                                                                           expected-warning {{variable length arrays are a Clang extension}} \
+                                                                           expected-note {{non-constexpr function 'strcmp' cannot be used in a constant expression}}
----------------
Another unfortunate case, similar to the error case earlier, of BOTH warning about about it being a variable-length but then also warning about it actually being constant array as an extension.


================
Comment at: clang/test/SemaCXX/offsetof.cpp:31
+  int array1[__builtin_offsetof(HasArray, array[i])]; // expected-warning {{variable length arrays are a Clang extension}} \
+                                                         new-interp-note {{function parameter 'i' with unknown value cannot be used in a constant expression}}
 }
----------------
Weird that new-interp adds the diagnostic for C++98 mode. I wonder if that indicates a bug (e.g. if in new-interp we accidentally use C++11 rules for C++98)?


================
Comment at: clang/test/SemaCXX/vla-ext-diag.cpp:13
+
+// FIXME: it's not clear why C++98 mode does not emit the extra notes in the
+// same way that C++11 mode does.
----------------
I think this is "expected" due to the divergence between the use of the constant-expression evaluation in C++11-and-later and "ICE" code in previous.


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

https://reviews.llvm.org/D156565