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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked an inline comment as done.
aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:147-149
+def ext_vla_cxx_static_assert : ExtWarn<
+  "variable length arrays in C++ are a Clang extension; did you mean to use "
+  "'static_assert'?">, InGroup<VLAUseStaticAssert>;
----------------
tahonermann wrote:
> I find the "did you mean to use 'static_assert'" phrasing awkward here since it is highly unlikely that someone intended to write `static_assert(x)` and instead wrote `int my_assert[x ? 1 : -1]`. Perhaps something like this?
> 
> "variable length arrays in C++ are a Clang extension; 'static_assert' can be used in this case".
Eh, I'm on the fence. We're consistent about asking users "did you mean X?" in our diagnostics and this follows the same pattern. "Did you mean to use X" is not "is this a typo for X?" but "were you aware you could do X instead?" So yeah, the wording is a bit awkward, but it's consistent with other diagnostics and not really wrong either. Do you have strong feelings?


================
Comment at: clang/lib/Sema/SemaType.cpp:2617
+  } else if (getLangOpts().CPlusPlus) {
+    if (getLangOpts().CPlusPlus11 && IsStaticAssertLike(ArraySize, Context))
+      VLADiag = getLangOpts().GNUMode
----------------
jyknight wrote:
> aaron.ballman wrote:
> > 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?
> In this patch, in C++98 mode, we diagnose everything under the flag -Wvla-extension, and never under the flag -Wvla-extension-static-assert. My point was that we ought to diagnose static-assert-like-VLAs under the -Wvla-extension-static-assert flag even in C++98 mode.
Ah okay! That does make sense but would be hard to pull off. Because the existing code is based around picking which diagnostic ID to use, all of the related diagnostic IDs need to take the same kind of streaming arguments, that makes passing additional arguments to control `%select` behavior really awkward in this case. So I'd have to add an entire new diagnostic ID for the C++98 case and I'm not certain it's really worth it. e.g.
```
def ext_vla : Extension<"variable length arrays are a C99 feature">,
  InGroup<VLAExtension>;
// In C++ language modes, we warn by default as an extension, while in GNU++
// language modes, we warn as an extension but add the warning group to -Wall.
def ext_vla_cxx : ExtWarn<
  "variable length arrays in C++ are a Clang extension">,
  InGroup<VLAExtension>;
def ext_vla_cxx_in_gnu_mode : Extension<ext_vla_cxx.Summary>,
  InGroup<VLAExtension>;
def ext_vla_cxx_static_assert : ExtWarn<
  "variable length arrays in C++ are a Clang extension; did you mean to use "
  "'static_assert'?">, InGroup<VLAUseStaticAssert>;
def ext_vla_cxx_in_gnu_mode_static_assert : Extension<
  ext_vla_cxx_static_assert.Summary>, InGroup<VLAUseStaticAssert>;
def ext_vla_cxx98_static_assert : ExtWarn<
  ext_vla_cxx.Summary>, InGroup<VLAUseStaticAssert>;
def ext_vla_cxx98_in_gnu_mode_static_assert : Extension<
  ext_vla_cxx98_static_assert.Summary>, InGroup<VLAUseStaticAssert>;
def warn_vla_used : Warning<"variable length array used">,
  InGroup<VLA>, DefaultIgnore;
```
It's not the end of the world, but do you think it's worth it?


================
Comment at: clang/test/SemaCXX/vla-ext-diag.cpp:34
+                                   off-note {{function parameter 'n' with unknown value cannot be used in a constant expression}}
+}
----------------
tahonermann wrote:
> Perhaps add a test where the conditional expression is parenthesized.
>   int array4[(n ? 1 : -1)]; 
> 
> Adding tests for one side being of size 0 might be useful to demonstrate that those are not intended to trigger the "use 'static_assert'" diagnostic. I know some compilers don't diagnose on a size of 0 and so the static assert idiom generally requires a negative number.
>   int array5[n ? 1 : 0]; // ok?
Good call on the additional test cases!


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

https://reviews.llvm.org/D156565