efriedma 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
}
----------------
aaron.ballman wrote:
> 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.)
I think there's some value in having flags for both "stack-allocated variable
length array" and "any type spelled using a VLA". Which one of those is
spelled "-Wvla" is up for debate, I guess. I'd be inclined to let the the
existing warning flag continue to work the way it has for the past; both clang
and gcc have supported it with the existing semantics for a long time. It's
not like there's any shortage of flag names.
When I was referring to "codebases compiled with both clang and MSVC", I wasn't
specifically referring to codebases that use clang on Windows; I was also
referring to codebases that use MSVC on Windows, and clang on other targets.
How -fms-compatibility works is a separate subject.
> the purpose to -Wvla is to identify nonportable code
The standard committee can't magically make something "portable"; portability
depends on implementations.
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
