Page MenuHomePhabricator

[OpenCL] Allow variadic macros as Clang feature
ClosedPublic

Authored by Anastasia on Mar 18 2019, 8:01 AM.

Details

Reviewers
bader
arsenm
Summary

Variadic macros are used for debugging and therefore it is desirable to accept the code that uses them.

There was a discussion about this in OpenCL spec bugs: https://github.com/KhronosGroup/OpenCL-Docs/issues/50

There was no disagreement on relaxing this , but however we didn't find a way to version this yet.

For now we are allowing this as Clang feature (difference to a standard).

I assume it's not a separate feature and therefore not worth to start Clang extension for this. So I just modified the Clang manual for now. However, I could also start a new section in the extension document: https://clang.llvm.org/docs/LanguageExtensions.html

Diff Detail

Event Timeline

Anastasia created this revision.Mar 18 2019, 8:01 AM
Anastasia updated this revision to Diff 191093.Mar 18 2019, 8:03 AM

Better wording

Anastasia updated this revision to Diff 191094.Mar 18 2019, 8:04 AM

Fixed comment

bader accepted this revision.Mar 18 2019, 8:17 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Mar 18 2019, 8:17 AM
arsenm added a subscriber: arsenm.Mar 18 2019, 2:07 PM

Should it be downgraded to a warning about an extension instead of just removing it?

Should it be downgraded to a warning about an extension instead of just removing it?

What would you suggest to put in a warning message? Clang normally doesn't warn about extensions...

Should it be downgraded to a warning about an extension instead of just removing it?

What would you suggest to put in a warning message? Clang normally doesn't warn about extensions...

Isn't that what -pedantic is for?

Should it be downgraded to a warning about an extension instead of just removing it?

What would you suggest to put in a warning message? Clang normally doesn't warn about extensions...

Isn't that what -pedantic is for?

Yes, indeed I think it's a better approach now since it's still in the spec...

Instead of removing the diagnostic completely change into a warning in pedantic mode.

arsenm added inline comments.Mar 20 2019, 11:29 AM
include/clang/Basic/DiagnosticLexKinds.td
397

Maybe rephrase the message now to say it's an extension? The other similar warnings seem to mention "GNU extension" or "Microsoft extensions" or <standard version> extension.

None of them seem to mention "clang" extensions however.

Anastasia updated this revision to Diff 192082.Mar 25 2019, 5:19 AM

Changed diagnostic wording to indicate that this feature is a Clang extension.

Anastasia marked an inline comment as done.Mar 25 2019, 5:20 AM
Anastasia added inline comments.
include/clang/Basic/DiagnosticLexKinds.td
397

Actually I have found a couple of diagnostics with Clang extension so it should be fine.

Thanks for the feedback.

arsenm accepted this revision.Mar 25 2019, 10:39 AM

LGTM

Anastasia closed this revision.Mar 26 2019, 11:11 AM

Committed in r356987.