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

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.


================
Comment at: clang/lib/Sema/SemaType.cpp:2617
+  } else if (getLangOpts().CPlusPlus) {
+    if (getLangOpts().CPlusPlus11 && IsStaticAssertLike(ArraySize, Context))
+      VLADiag = getLangOpts().GNUMode
----------------
aaron.ballman wrote:
> 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?
Yea, that's basically what I meant.

But, actually, the message is not great in C++11 mode either -- "use static_assert" isn't really what's meant, since the only reason it's diagnosed here at all is because the value (accidentally or intentionally) _isn't_ actually a constant expression and thus wouldn't work under static_assert.

A message like: `variable length arrays in C++ are a Clang extension; this looks like it was intended to be a pre-C++11 emulation of static_assert, but doesn't have a constant condition` would be correct in both language modes, and maybe be more to the point for the issue.



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

https://reviews.llvm.org/D156565