[PATCH] D156565: Diagnose use of VLAs in C++ by default

aaron.ballman added a comment. In D156565#4654820 , @cor3ntin wrote: >> Any concerns with this approach? > > Sounds reasonable to me Thanks for the double-check! This should now be fixed in https://github.com/llvm/llvm-project/commit/f8d448d5e587a23886c

[PATCH] D156565: Diagnose use of VLAs in C++ by default

cor3ntin added a comment. > Any concerns with this approach? Sounds reasonable to me Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156565/new/ https://reviews.llvm.org/D156565 ___ cfe-commits mailing li

[PATCH] D156565: Diagnose use of VLAs in C++ by default

aaron.ballman added a comment. In D156565#4654818 , @aaron.ballman wrote: > In D156565#4654773 , @nathanchance > wrote: > >> Is it expected that this introduces a warning for C code, as the commit >> message an

[PATCH] D156565: Diagnose use of VLAs in C++ by default

aaron.ballman added a comment. In D156565#4654773 , @nathanchance wrote: > Is it expected that this introduces a warning for C code, as the commit > message and tests appear to only affect C++? A trivial example from the Linux > kernel: > > https://eli

[PATCH] D156565: Diagnose use of VLAs in C++ by default

nathanchance added a comment. Is it expected that this introduces a warning for C code, as the commit message and tests appear to only affect C++? A trivial example from the Linux kernel: https://elixir.bootlin.com/linux/v6.5.8/source/tools/lib/bpf/btf_dump.c#L1678 #include #include #in

[PATCH] D156565: Diagnose use of VLAs in C++ by default

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG7339c0f782d5: Diagnose use of VLAs in C++ by default (authored by aaron.ballman). Changed prior to commit: https://reviews.llvm.org/D156565?vs=557

[PATCH] D156565: Diagnose use of VLAs in C++ by default

aaron.ballman added a comment. Ping 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

[PATCH] D156565: Diagnose use of VLAs in C++ by default

aaron.ballman updated this revision to Diff 557575. aaron.ballman added a comment. Updated based on review feedback. Specifically: - Added test cases, fixed handling of parenthesized expressions. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156565/new/ https://reviews.llvm.org/D156565

[PATCH] D156565: Diagnose use of VLAs in C++ by default

aaron.ballman marked an inline comment as not done. aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:2617 + } else if (getLangOpts().CPlusPlus) { +if (getLangOpts().CPlusPlus11 && IsStaticAssertLike(ArraySize, Context)) + VLADiag = getLangOpt

[PATCH] D156565: Diagnose use of VLAs in C++ by default

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: > jykni

[PATCH] D156565: Diagnose use of VLAs in C++ by default

tahonermann 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; aaron

[PATCH] D156565: Diagnose use of VLAs in C++ by default

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 " + "'

[PATCH] D156565: Diagnose use of VLAs in C++ by default

tahonermann 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; I fin

[PATCH] D156565: Diagnose use of VLAs in C++ by default

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: > jykni

[PATCH] D156565: Diagnose use of VLAs in C++ by default

tbaeder added inline comments. 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

[PATCH] D156565: Diagnose use of VLAs in C++ by default

cor3ntin accepted this revision. cor3ntin added a comment. This revision is now accepted and ready to land. LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156565/new/ https://reviews.llvm.org/D156565 ___ cfe-commits mailing list cfe-comm

[PATCH] D156565: Diagnose use of VLAs in C++ by default

aaron.ballman updated this revision to Diff 557531. aaron.ballman marked 4 inline comments as done. aaron.ballman added a comment. Updated based on review feedback. Specifically: - Updated a FIXME comment in a test to clarify what's happening. - Reworded diagnostic to add "in C++" CHANGES SINCE

[PATCH] D156565: Diagnose use of VLAs in C++ by default

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

[PATCH] D156565: Diagnose use of VLAs in C++ by default

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; +def ext_vla_cxx_in_gnu_mode : Exten

[PATCH] D156565: Diagnose use of VLAs in C++ by default

aaron.ballman updated this revision to Diff 557483. aaron.ballman added a comment. Updated based on feedback on the RFC. Specifically, this variant recognizes `int array[cond ? 1 : -1];` or similar constructs and recommends using a `static_assert` instead. This new diagnostic is under its own wa

[PATCH] D156565: Diagnose use of VLAs in C++ by default

aaron.ballman added a comment. I put up an RFC at https://discourse.llvm.org/t/rfc-diagnosing-use-of-vlas-in-c/73109 to reach a slightly wider audience for awareness. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156565/new/ https://reviews.llvm.org/D156565 __

[PATCH] D156565: Diagnose use of VLAs in C++ by default

cor3ntin added a comment. In D156565#4605027 , @shafik wrote: > In D156565#4599812 , @aaron.ballman > wrote: > >> Enable the diagnostic by default in C++ language modes, and under -Wall in >> GNU++ language mode

[PATCH] D156565: Diagnose use of VLAs in C++ by default

shafik added a comment. In D156565#4599812 , @aaron.ballman wrote: > Enable the diagnostic by default in C++ language modes, and under -Wall in > GNU++ language modes. I am happy with that approach. CHANGES SINCE LAST ACTION https://reviews.llvm.or

[PATCH] D156565: Diagnose use of VLAs in C++ by default

aaron.ballman updated this revision to Diff 551596. aaron.ballman added a comment. Enable the diagnostic by default in C++ language modes, and under -Wall in GNU++ language modes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156565/new/ https://reviews.llvm.org/D156565 Files: clang

[PATCH] D156565: Diagnose use of VLAs in C++ by default

aaron.ballman added a comment. In D156565#4580716 , @Endill wrote: > In D156565#4547909 , @aaron.ballman > wrote: > >> In D156565#4543503 , >> @aaron.ballman wrote: >> >

[PATCH] D156565: Diagnose use of VLAs in C++ by default

Endill added a comment. In D156565#4547909 , @aaron.ballman wrote: > In D156565#4543503 , @aaron.ballman > wrote: > >> In D156565#4543414 , @jrtc27 wrote: >> >>> Given G

[PATCH] D156565: Diagnose use of VLAs in C++ by default

aaron.ballman added a comment. In D156565#4543503 , @aaron.ballman wrote: > In D156565#4543414 , @jrtc27 wrote: > >> Given GCC defines GNU C++ and regards this as a feature (unless you use >> things like -pedant

[PATCH] D156565: Diagnose use of VLAs in C++ by default

aaron.ballman added a comment. In D156565#4543414 , @jrtc27 wrote: > Given GCC defines GNU C++ and regards this as a feature (unless you use > things like -pedantic to ask for ISO C++), does it make sense to enable this > for GNU C++? I think GCC shoul

[PATCH] D156565: Diagnose use of VLAs in C++ by default

jrtc27 added a comment. Given GCC defines GNU C++ and regards this as a feature (unless you use things like -pedantic to ask for ISO C++), does it make sense to enable this for GNU C++? Every deviation from GCC is a point of friction for adopting Clang over GCC. Repository: rG LLVM Github M

[PATCH] D156565: Diagnose use of VLAs in C++ by default

aaron.ballman added subscribers: ABataev, clang-vendors. aaron.ballman added a comment. The patch seems large and scary, but that's because of how often we've had to test VLA behavior in C++ code. The OpenMP test cases are mostly opting out of VLA warnings mechanically (I verified with @ABataev

[PATCH] D156565: Diagnose use of VLAs in C++ by default

aaron.ballman created this revision. aaron.ballman added reviewers: rsmith, erichkeane, clang-language-wg, hubert.reinterpretcast, shafik, jyknight. Herald added subscribers: mattd, pmatos, asb, asavonic, jdoerfert, sbc100, dschuff. Herald added a project: All. aaron.ballman requested review of t