[PATCH] D132952: [Sema] disable -Wvla for function array parameters

[Aaron Ballman via Phabricator via cfe-commits]

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 at -2{{variable length arrays are a C99 feature}}
+#endif
 }
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132952/new/

https://reviews.llvm.org/D132952