[PATCH] D123955: [C2x] Disallow functions without prototypes/functions with identifier lists

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.

LGTM. Some comments for potential improvements, but I'd be OK with this landing as-is if you don't find any of them sufficiently motivating :)



================
Comment at: clang/lib/Parse/ParseDecl.cpp:6664-6666
+    // OpenCL disallows variadic functions, so it also disallows a function
+    // without a prototype. However, it doesn't enforce strict prototypes
+    // because it allows function definitions with an identifier list.
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > I don't follow this comment: functions without a prototype are not variadic (they're compatible with any *non-variadic* prototype), so OpenCL disallowing variadic functions seems irrelevant here.
> > Heh, this comment came from feedback I got from someone on IRC when I was asking what OpenCL actually supports. As best I found, OpenCL allows `void f();` to mean `void f(void);` as in C++, but also allows `void f(a, b) int a, b; {}` (despite having no way to actually declare this function).
> > 
> > I'll take a pass at fixing up the comment to be more clear, thanks!
> I updated both comments.
Fascinating! There's a common confusion and misapprehension that `void f()` declares a variadic function in C, and that confusion has made its way into the OpenCL specification. The relevant rule there is 6.11/g:

> e. Variadic functions are not supported, with the exception of printf and enqueue_kernel.
> g. If a list of parameters in a function declaration is empty, the function takes no arguments. **This is due to the above restriction on variadic functions.**

(Emphasis mine.) So I think this implementation is correct, and your earlier comment correctly reflected the confusion in the OpenCL spec :-)

A comment with a direct reference to this part of the [OpenCL C 3.0 specification](https://www.khronos.org/registry/OpenCL/specs/3.0-unified/pdf/OpenCL_C.pdf) would be nice.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:6667
+    // have an identifier list.
+    HasProto = ParamInfo.size() || getLangOpts().requiresStrictPrototypes() ||
+               getLangOpts().OpenCL;
----------------
Hm, so `-fstrict-prototypes` causes us to treat `void f()` as `void f(void)`? That's not normally how our `-fstrict-` flags work: normally they mean "strictly enforce this language rule, even though that may result in programs having UB that we could define away with a more permissive rule". (For example, `-fstrict-aliasing`, `-fstrict-float-cast-overflow`, `-fstrict-enums`, `-fstrict-overflow`, `-fstrict-vtable-pointers`, `-fstrict-return` all work like that.) I wonder if a different flag name would work better, eg `-fno-unprototyped-functions`. Is `-fstrict-prototypes` a GCC flag that we're trying to be compatible with, or our own invention?

If you can't find a better name, I'm not dead set against the current one, but it does seem a little inconsistent.


================
Comment at: clang/test/Driver/strict-prototypes.c:5
+// RUN: not %clang -fno-strict-prototypes -x c %s 2>&1 | FileCheck %s
+// RUN: not %clang -fno-strict-prototypes -std=c89 -x c %s 2>&1 | FileCheck %s
+
----------------
I would expect this case (`-std=c89 -fno-strict-prototypes`) to work: we usually allow ` -fno-X` flag to override an earlier `-fX` flag in the same command line. Not a big deal, though.


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

https://reviews.llvm.org/D123955