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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 4 inline comments as done.
aaron.ballman added a subscriber: tbaeder.
aaron.ballman added inline comments.


================
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>,
----------------
jyknight wrote:
> 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.
> Clarify text: "variable length arrays in C++ are a Clang extension" and same below.

Sure, that's reasonable, thanks!

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

Agreed that we should be consistent, but the standard term of art is `variable length array` despite "variable-length array" being arguably more correct, and that usage is the more common in Clang (273 uses of "variable length array" compared to 64 uses of "variable-length array").


================
Comment at: clang/lib/Sema/SemaType.cpp:2617
+  } else if (getLangOpts().CPlusPlus) {
+    if (getLangOpts().CPlusPlus11 && IsStaticAssertLike(ArraySize, Context))
+      VLADiag = getLangOpts().GNUMode
----------------
jyknight wrote:
> 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?
That's effectively what we're doing here, right?

C++98 is told "variable length arrays (in C++) are a Clang extension" and in C++11 they're told "variable length arrays (in C++) are a Clang extension; did you mean to use 'static_assert'?"

Or am I misunderstanding you?


================
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}}
----------------
jyknight wrote:
> 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.
> 
Yeah, agreed that this is unfortunate, but it comes from the split between the type and the declaration checking code and that's going to be hard to work around.


================
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}}
----------------
jyknight wrote:
> 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.
Yup, same kind of issue here as above in terms of the cause.


================
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}}
 }
----------------
jyknight wrote:
> 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)?
CC @tbaeder 


================
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.
----------------
jyknight wrote:
> 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.
Ah you're exactly right about that, thank you!


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

https://reviews.llvm.org/D156565