aaron.ballman marked 4 inline comments as done.
aaron.ballman added a subscriber: tbaeder.
aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:143
+def ext_vla_cxx : ExtWarn<
+ "variable length arrays are a Clang extension">, InGroup<VLAExtension>;
+def ext_vla_cxx_in_gnu_mode : Extension<ext_vla_cxx.Summary>,
----------------
jyknight wrote:
> Clarify text: "variable length arrays in C++ are a Clang extension" and same
> below.
>
> I also think we should be (consistently) using the term "variable-length
> array" with a dash instead of "variable length array", but if you change
> that, it should be in a follow-on which changes it in all the messages.
> Clarify text: "variable length arrays in C++ are a Clang extension" and same
> below.
Sure, that's reasonable, thanks!
> I also think we should be (consistently) using the term "variable-length
> array" with a dash instead of "variable length array", but if you change
> that, it should be in a follow-on which changes it in all the messages.
Agreed that we should be consistent, but the standard term of art is `variable
length array` despite "variable-length array" being arguably more correct, and
that usage is the more common in Clang (273 uses of "variable length array"
compared to 64 uses of "variable-length array").
================
Comment at: clang/lib/Sema/SemaType.cpp:2617
+ } else if (getLangOpts().CPlusPlus) {
+ if (getLangOpts().CPlusPlus11 && IsStaticAssertLike(ArraySize, Context))
+ VLADiag = getLangOpts().GNUMode
----------------
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?
================
Comment at: clang/test/CXX/dcl.dcl/dcl.spec/dcl.typedef/p2-0x.cpp:39
+ using T = int[n]; // expected-error {{variable length array declaration not
allowed at file scope}} \
+ expected-warning {{variable length arrays are a Clang
extension}} \
+ expected-note {{read of non-const variable 'n' is not
allowed in a constant expression}}
----------------
jyknight wrote:
> That we get a warning _and_ an error for this using statement now is
> non-ideal. I see that we are doing this in two different places,
> though...first in CheckType for the VLA, then diagnosing if it's used at file
> scope in CheckDecl. So maybe not worth fixing.
>
Yeah, agreed that this is unfortunate, but it comes from the split between the
type and the declaration checking code and that's going to be hard to work
around.
================
Comment at: clang/test/SemaCXX/cxx1z-noexcept-function-type.cpp:124
+ typedef int arr[strcmp("bar", "foo") + 4 * strncmp("foo", "bar", 4)]; //
expected-warning {{variable length array folded to constant array as an
extension}} \
+
expected-warning {{variable length arrays are a Clang extension}} \
+
expected-note {{non-constexpr function 'strcmp' cannot be used in a constant
expression}}
----------------
jyknight wrote:
> Another unfortunate case, similar to the error case earlier, of BOTH warning
> about about it being a variable-length but then also warning about it
> actually being constant array as an extension.
Yup, same kind of issue here as above in terms of the cause.
================
Comment at: clang/test/SemaCXX/offsetof.cpp:31
+ int array1[__builtin_offsetof(HasArray, array[i])]; // expected-warning
{{variable length arrays are a Clang extension}} \
+ new-interp-note
{{function parameter 'i' with unknown value cannot be used in a constant
expression}}
}
----------------
jyknight wrote:
> Weird that new-interp adds the diagnostic for C++98 mode. I wonder if that
> indicates a bug (e.g. if in new-interp we accidentally use C++11 rules for
> C++98)?
CC @tbaeder
================
Comment at: clang/test/SemaCXX/vla-ext-diag.cpp:13
+
+// FIXME: it's not clear why C++98 mode does not emit the extra notes in the
+// same way that C++11 mode does.
----------------
jyknight wrote:
> I think this is "expected" due to the divergence between the use of the
> constant-expression evaluation in C++11-and-later and "ICE" code in previous.
Ah you're exactly right about that, thank you!
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
