This is an archive of the discontinued LLVM Phabricator instance.

__uuidof() and declspec(uuid("...")) should be allowed on enumeration types
ClosedPublic

Authored by puetzk on Nov 18 2016, 4:42 AM.

Details

Summary

Although not specifically mentioned in the documentation, MSVC accepts __uuidof(…) and declspec(uuid("…")) attributes on enumeration types in addition to structs/classes. This is meaningful, as such types *do* have associated UUIDs in ActiveX typelibs, and such attributes are included by default in the wrappers generated by their #import construct, so they are not particularly unusual.

clang currently rejects the declspec with a –Wignored-attributes warning, and errors on uuidof() with “cannot call operator uuidof on a type with no GUID” (because it rejected the uuid attribute, and therefore finds no value). This is causing problems for us while trying to use clang-tidy on a codebase that makes heavy use of ActiveX.

I believe I have found the relevant places to add this functionality, this patch adds this case to clang’s implementation of these MS extensions.
patch is against r285994 (or actually the git mirror 80464680ce).

Both include an update to test/Parser/MicrosoftExtensions.cpp to exercise the new functionality.

This is my first time contributing to LLVM, so if I’ve missed anything else needed to prepare this for review just let me know!

__uuidof: https://msdn.microsoft.com/en-us/library/zaah6a61.aspx
declspec(uuid("…")): https://msdn.microsoft.com/en-us/library/3b6wkewa.aspx
#import: https://msdn.microsoft.com/en-us/library/8etzzkb6.aspx

Diff Detail

Repository
rL LLVM

Event Timeline

puetzk updated this revision to Diff 78499.Nov 18 2016, 4:42 AM
puetzk retitled this revision from to __uuidof() and declspec(uuid("...")) should be allowed on enumeration types.
puetzk updated this object.
puetzk set the repository for this revision to rL LLVM.
puetzk added a subscriber: cfe-commits.

Do we have a testcase where the declspec is applied to something inappropriate like an int?

lib/Sema/SemaDeclAttr.cpp
4669–4673 ↗(On Diff #78499)

I don't think you need this now that you've got this in Attr.td

lib/Sema/SemaExprCXX.cpp
523 ↗(On Diff #78499)

Please renamed RD to something more appropriate like TD.

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added inline comments.
include/clang/Basic/Attr.td
1621 ↗(On Diff #78499)

You can drop the WarnDiag and ExpectedEnumOrClass -- that should be handled automatically by the tablegen emitter once you change CalculateDiagnostic() in ClangAttrEmitter.cpp to handle it properly.

include/clang/Basic/DiagnosticSemaKinds.td
2637 ↗(On Diff #78499)

This should read classes and enumerations.

lib/Sema/SemaDeclAttr.cpp
4669–4673 ↗(On Diff #78499)

Correct, this entire block should go away.

puetzk updated this revision to Diff 78569.Nov 18 2016, 12:26 PM

Address comments from @aaron.ballman and @majnemer

Add a test case wihich triggers "the 'uuid' attribute only applies to classes and enumerations" warning

Also rebased against r287335/496a3f56c7

puetzk marked 5 inline comments as done.Nov 18 2016, 1:27 PM
majnemer edited edge metadata.Dec 7 2016, 12:00 PM

This LGTM but Aaron should give the go ahead.

rnk accepted this revision.Dec 12 2016, 9:42 AM
rnk edited edge metadata.

This looks good to me, I'll merge this today unless I hear otherwise.

This revision is now accepted and ready to land.Dec 12 2016, 9:42 AM
This revision was automatically updated to reflect the committed changes.
rnk added a comment.Dec 13 2016, 11:08 AM

FYI I had to make a few changes so that we still error out on __uuid in C mode.

I see that you added a FIXME mentioning that it should be C++-only, but I don't see where you actually did anything that would make it so. What am I missing?

Also, did you mean to drop the changes to utils/TableGen/ClangAttrEmitter.cpp and bring back the WarnDiag, "ExpectedEnumOrClass" @aaron.ballman asked for in the first review?