aaron.ballman marked an inline comment as not done.
aaron.ballman added inline comments.
================
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:
> > > 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.
>
> 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.
Good point!!
> 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.
Oof, that's a mouthful. Stepping back a bit, I think the only reason we want a
second diagnostic group here is so people can be alerted to using a VLA while
turning off diagnostics for static_assert emulation. From that perspective, we
don't need different wording for the diagnostics so much as different
diagnostic groups for the same diagnostic wording. e.g.,
```
void old_style_static_assert(int n) {
int array1[n != 12 ? 1 : -1]; // warning: variable length arrays in C++ are a
Clang extension [-Wvla-extension-static-assert]
int array2[n]; // warning: variable length arrays in C++ are a Clang
extension [-Wvla-extension]
}
```
Do you think that could be reasonable behavior, or is that too subtle?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156565/new/
https://reviews.llvm.org/D156565
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
