aaron.ballman marked 2 inline comments as done.
aaron.ballman added inline comments.
================
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.
----------------
rsmith wrote:
> 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.
> 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.
I think it's understandable confusion. `void f(...);` in C++ and `void f();` in
C89 say the "same" thing: this function can be called with zero or more
arguments, YOLO. The fact that the standards made `...` mean "variadic" but
functions without a prototype mean "magic" is mostly lost on users, I think.
> A comment with a direct reference to this part of the OpenCL C 3.0
> specification would be nice.
I'll add that link in, good call!
================
Comment at: clang/lib/Parse/ParseDecl.cpp:6667
+ // have an identifier list.
+ HasProto = ParamInfo.size() || getLangOpts().requiresStrictPrototypes() ||
+ getLangOpts().OpenCL;
----------------
rsmith wrote:
> 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.
> Hm, so -fstrict-prototypes causes us to treat void f() as void f(void)?
Yup, the idea is that it is strictly enforcing that all functions have a
prototype.
> Is -fstrict-prototypes a GCC flag that we're trying to be compatible with,
> or our own invention?
It's our invention, and I'm not strongly tied to the name. It's the one that
came up during the RFC and I suspect it's influenced by the name of the warning
group `-Wstrict-prototypes`.
I think `-fno-` would be a bit of a weird way to spell the flag as that usually
disables something rather than enables it. I'll switch to `-fforce-prototypes`
because that's basically what this does. WDYT?
================
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
+
----------------
rsmith wrote:
> 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.
I think it's better to disallow the user from ever uttering
`-fno-strict-prototypes` even when it's behavior would be benign. But I'm happy
to revisit later if it becomes an issue.
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
