Page MenuHomePhabricator

[OpenCL] Allow function declaration with empty argument list.
ClosedPublic

Authored by bader on May 30 2017, 8:49 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

bader created this revision.May 30 2017, 8:49 AM
yaxunl edited edge metadata.May 30 2017, 11:19 AM

I kind of like this change. In reality, people often use f() as an alternative of f(void). I think diagnose it as an error is just causing inconvenience.

Anastasia edited edge metadata.May 31 2017, 7:26 AM

I kind of like this change. In reality, people often use f() as an alternative of f(void). I think diagnose it as an error is just causing inconvenience.

Yes, I also think it's useful in general. Just wondering whether we should propagate it to Khronos too since it's a deviation from either C99 or OpenCL spec.

bader added a comment.Jun 2 2017, 1:31 AM

Sorry for the delay.
It will also align the behavior with function definition.
For instance the following function definition is allowed.

__local int *allocateLocalMemory() { return 0; }

Regarding propagating this to Khronos, I'm not sure if this feature requires additional clarification in the specification.
My understanding is that it's clear that 'foo()' is equivalent to 'foo(void)'.
Do you have any particular proposal to the specification text in mind?

Sorry for the delay.
It will also align the behavior with function definition.
For instance the following function definition is allowed.

__local int *allocateLocalMemory() { return 0; }

Regarding propagating this to Khronos, I'm not sure if this feature requires additional clarification in the specification.
My understanding is that it's clear that 'foo()' is equivalent to 'foo(void)'.
Do you have any particular proposal to the specification text in mind?

We could steal some text from C++? Or we can create a ticket to Khronos and see if they resolve this. Otherwise, I am also ok to keep this as Clang "extension".

Should we add cfe-commits then?

Anastasia accepted this revision.Jun 7 2017, 1:43 PM

LGTM! Thanks!

This revision is now accepted and ready to land.Jun 7 2017, 1:43 PM
bader updated this revision to Diff 102319.Jun 13 2017, 5:45 AM

Update one more test missed in the first version of the patch.

Actually it looks like a bug in Parser.
Clang interprets following statement as variable declaration.

extern pipe write_only int get_pipe();

So the type of the pipe element is function prototype: int get_pipe().

If I understand the intention correctly, clang is expected to treat this expression function declaration with pipe return type, which elements are int.

Would it acceptable to commit this patch as is and file another bug on Parser?
I need more time to think how to resolve this ambiguity.

bader retitled this revision from Allow function declaration with empty argument list. to [OpenCL] Allow function declaration with empty argument list..Jun 13 2017, 5:47 AM
bader added a comment.Jun 23 2017, 7:43 AM

Ping.

Although this patch is already accepted, I'd like to confirm that I can commit the latest update with changes in test/SemaOpenCL/invalid-pipes-cl2.0.cl.

Thanks.

Sure, no problem! We might add some note on this feature to Clang manual. But we can do it later as well. Thanks!

bader closed this revision.Jun 29 2017, 1:44 AM
This revision is now accepted and ready to land.Jun 29 2017, 3:56 AM
bader added a comment.Jun 29 2017, 5:05 AM

@chapuni, thanks for taking care of this. I'll take a look.

bader updated this revision to Diff 118210.Oct 9 2017, 8:37 AM
bader edited the summary of this revision. (Show Details)

Fix parsing of function declarations with empty parameter list initializers.
This change should fix the failure caught by the buildbot.
Sorry for the long delay.

Anastasia accepted this revision.Oct 10 2017, 10:08 AM

I would prefer to merge the new test with test/SemaOpenCL/func.cl, but otherwise LGTM! Thanks!

yaxunl accepted this revision.Oct 10 2017, 10:52 AM

LGTM. Thanks.

bader updated this revision to Diff 118437.Oct 10 2017, 11:31 AM

Applied comments.

This revision was automatically updated to reflect the committed changes.