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