Page MenuHomePhabricator

[OpenCL]Allowing explicit conversion of "0" to event_t type
ClosedPublic

Authored by ashi1 on Feb 24 2016, 12:04 PM.

Diff Detail

Event Timeline

ashi1 updated this revision to Diff 48966.Feb 24 2016, 12:04 PM
ashi1 retitled this revision from to [OpenCL]Allowing explicit conversion of "0" to event_t type.
ashi1 updated this object.
ashi1 added reviewers: Anastasia, pxli168, yaxunl.
pxli168 edited edge metadata.Feb 24 2016, 10:42 PM
foo((event_t)0);

Is above use have been hint by some test cases?

include/clang/Basic/DiagnosticSemaKinds.td
7679

Please do not use DOS format.

lib/Sema/SemaCast.cpp
2316
  1. Comments start with capital.
  2. End a sentence with a period.
  3. Give spec reference here, start with:

OpenCL v2.0 s6.13.10 - ....

2317

(Self.getLangOpts().OpenCL) -> Self.getLangOpts().OpenCL

2318

Indent.

2319

Sema has a context.
Self.getASTContext() ->Self.Context

2321

There is a CK_ZeroToOCLEvent, why not use that?

2324–2328

Indent.
You can try to use clang-format to help keep these right.

pxli168 added inline comments.Feb 25 2016, 12:43 AM
lib/Sema/SemaCast.cpp
2324–2326

Need a test case for this.

Anastasia edited edge metadata.Feb 26 2016, 2:29 AM

Could you please change my role to the Subscriber and add Joey as a reviewer please.

ashi1 marked 7 inline comments as done.Feb 26 2016, 8:37 AM
ashi1 added a subscriber: ashi1.

Joey Gouly or Joey Korkames?

Thanks,
Aaron

ashi1 edited subscribers, added: Anastasia; removed: ashi1.

Joey Gouly!

Thanks!

ashi1 added a reviewer: joey.Feb 26 2016, 8:50 AM
ashi1 updated this revision to Diff 49219.Feb 26 2016, 12:13 PM
ashi1 marked an inline comment as done.

Revised with Xiuli Pan's comments.

lib/Sema/SemaCast.cpp
2317

A similar patch has been submitted and rejected several times:

https://www.mail-archive.com/cfe-commits@cs.uiuc.edu/msg91067.html

http://marc.info/?l=cfe-commits&m=141198505414824&w=2

These were all before OpenCL 2.0. The 1.2 spec does not allow this, so I think this should be guarded by an OpenCL version check.

yaxunl added inline comments.Feb 26 2016, 1:13 PM
lib/Sema/SemaCast.cpp
2317

I opened a bug at khronos bugzilla requesting clarification of the issue:

https://cvs.khronos.org/bugzilla/show_bug.cgi?id=15609

I will keep you updated.

yaxunl added inline comments.May 5 2016, 1:51 PM
lib/Sema/SemaCast.cpp
2317

OpenCL WG has clarified that explicit or implicit cast of 0 to event_t is allowed. It does not make sense to only allow implicit cast but not allow explicit cast. So I think we should move on with this patch.

ashi1 marked 5 inline comments as done.May 9 2016, 11:03 AM
ashi1 updated this revision to Diff 56599.May 9 2016, 11:07 AM

Added changes for comments from majnemer.

ashi1 updated this revision to Diff 56615.May 9 2016, 1:12 PM

Modified the testcase. Tested this diff on clang-test and it passes.

joey accepted this revision.May 11 2016, 1:36 AM
joey edited edge metadata.

LGTM!

This revision is now accepted and ready to land.May 11 2016, 1:36 AM
yaxunl accepted this revision.May 11 2016, 8:11 AM
yaxunl edited edge metadata.

LGTM. Thanks!

Xiuli, are you OK with this patch?

pxli168 accepted this revision.May 19 2016, 9:49 PM
pxli168 edited edge metadata.

Sorry about late reply.
LGTM!

This revision was automatically updated to reflect the committed changes.