aaron.ballman added inline comments.
================ Comment at: clang/test/Sema/warn-vla.c:8-12 +void test2(int n, int v[n]) { // c99 no-warning +#if __STDC_VERSION__ < 199901L +// expected-warning@-2{{variable length arrays are a C99 feature}} +#endif } ---------------- efriedma wrote: > aaron.ballman wrote: > > efriedma wrote: > > > aaron.ballman wrote: > > > > efriedma wrote: > > > > > aaron.ballman wrote: > > > > > > aaron.ballman wrote: > > > > > > > inclyc wrote: > > > > > > > > aaron.ballman wrote: > > > > > > > > > The diagnostic there is rather unfortunate because we're not > > > > > > > > > using a variable-length array in this case. > > > > > > > > Emm, I'm not clear about whether we should consider this a VLA, > > > > > > > > and generates `-Wvla-extensions`. Is `v[n]` literally a > > > > > > > > variable-length array? (in source code) So it seems to me that > > > > > > > > we should still report c89 incompatibility warnings? > > > > > > > > > > > > > > > C89's grammar only allowed for an integer constant expression to > > > > > > > (optionally) appear as the array extent in an array declarator, > > > > > > > so there is a compatibility warning needed for that. But I don't > > > > > > > think we should issue a warning about this being a VLA in C99 or > > > > > > > later. The array *is* a VLA in terms of the form written in the > > > > > > > source, but C adjusts the parameter to be a pointer parameter, so > > > > > > > as far as the function's type is concerned, it's not a VLA (it's > > > > > > > just a self-documenting interface). > > > > > > > > > > > > > > Because self-documenting code is nice and because people are > > > > > > > worried about accidental use of VLAs that cause stack allocations > > > > > > > (which this does not), I think we don't want to scare people off > > > > > > > from this construct. But I'm curious what others think as well. > > > > > > > But I'm curious what others think as well. > > > > > > > > > > > > (Specifically, I'm wondering if others agree that the only warning > > > > > > that should be issued there is a C89 compatibility warning under > > > > > > `-Wvla-extensions` when in C89 mode and via a new `CPre99Compat` > > > > > > diagnostic group when enabled in C99 and later modes.) > > > > > > > > > > > > > > > > > I imagine people working with codebases that are also built with > > > > > compilers that don't support VLAs would still want some form of > > > > > warning on any VLA type. > > > > The tricky part is: that's precisely what WG14 N2992 > > > > (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2992.pdf) is > > > > clarifying. If your implementation doesn't support VLAs, it's still > > > > required to support this syntactic form. So the question becomes: do we > > > > want a portability warning to compilers that don't conform to the > > > > standard? Maybe we do (if we can find evidence of compilers in such a > > > > state), but probably under a specific diagnostic flag rather than -Wvla. > > > That only applies to C23, though, right? None of that wording is there > > > for C11. (In particular, MSVC claims C11 conformance without any support > > > for VLA types.) > > In a strict sense, yes, this is a C23 change. There are no changes to older > > standards as far as ISO is concerned (and there never have been). > > > > In a practical sense, no, this is clarifying how VLAs have always been > > intended to work. C23 is in a really weird spot in that we had to stop our > > "defect report" process at the end of C17 because the ISO definition of a > > defect did not match the WG14 definition of a defect, and ISO got upset. > > Towards the tail end of the C23 cycle, we introduced a list of extensions > > to obsolete versions of C > > (https://www.open-std.org/jtc1/sc22/wg14/www/previous.html) as a stopgap, > > but the convener would not allow us to retroactively add papers to it. The > > committee still hasn't gotten used to this new process and as a result, not > > many papers have made it to the list. We've also not had the chance to > > discuss https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3002.pdf on > > having a more reasonable issue tracking process. > > > > The end result is that there's a whole pile of papers in C23 that we would > > have normally treated via the old defect report process but couldn't, and > > if we had treated them via the old DR process, it would have been more > > clear why I think this should be a retroactive change back to C99 instead > > of a C23-only change. > Even if the committee can make retroactive changes to standards, that doesn't > change the fact that MSVC doesn't support VLAs, and that codebases exist > which need to be compiled with both clang and MSVC. Such code bases are already supported: https://godbolt.org/z/7n768WKW7 but I take your point that not everyone writes portable code when trying to port between compilers, and so some form of diagnostic would be nice. But I don't think that diagnostic is spelled `-Wvla` because the whole point of the paper was to clarify that such a function signature is mandatory to support even if you claim you don't support VLAs and the purpose to `-Wvla` is to identify nonportable code because it's using a VLA. Alternatively, when in `-fms-compatibility` mode, we could disallow VLAs and function signatures which involve one because that's not compatible with MSVC. (No idea just how much code that would break or whether I think this idea has much merit or not, just observing that VLAs are a weird boundary case in terms of MS compatibility because they're not a language extension.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132952/new/ https://reviews.llvm.org/D132952 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits