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