jyknight added a comment.
Overall looks good, just a few nits from looking things over.
================
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>,
----------------
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.
================
Comment at: clang/lib/Sema/SemaType.cpp:2617
+ } else if (getLangOpts().CPlusPlus) {
+ if (getLangOpts().CPlusPlus11 && IsStaticAssertLike(ArraySize, Context))
+ VLADiag = getLangOpts().GNUMode
----------------
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?
================
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}}
----------------
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.
================
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}}
----------------
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.
================
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}}
}
----------------
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)?
================
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.
----------------
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.
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
