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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
