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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
