[PATCH] D87652: Sema: add support for `__attribute__((__swift_newtype__))`

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added subscribers: dexonsmith, doug.gregor.
compnerd added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:2179
+def SwiftNewType : InheritableAttr {
+  let Spellings = [GNU<"swift_newtype">, GNU<"swift_wrapper">];
+  let Args = [EnumArgument<"NewtypeKind", "NewtypeKind",
----------------
aaron.ballman wrote:
> We don't ever document `swift_wrapper`, is that intentional?
> 
> (Do you have code elsewhere that's looking at the spelling of the AST attribute object? Should you add an `AdditionalMembers` to this declaration to make that easier to distinguish?)
Yes, that was intentional.  I believe that `swift_wrapper` predates `swift_newtype` and is only kept for compatibility.  People should gravitate towards `swift_newtype`.

I don't understand the need for `AdditionalMembers`.


================
Comment at: clang/include/clang/Basic/Attr.td:2183
+  let Subjects = SubjectList<[TypedefName], ErrorDiag>;
+  let Documentation = [SwiftNewTypeDocs];
+}
----------------
aaron.ballman wrote:
> You should also set: `let HasCustomParsing = 1;` since this uses a custom parser.
> 
> Is the custom parser actually needed though? Or is it only needed because the enum names selected are keywords? If it's only because of the keyword nature, should the parser be improved in `Parser::ParseAttributeArgsCommon()` instead?
It's for the keyword handling.  I'll take a look and see if I can convert this to the `ParseAttributeArgsCommon` path.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:4038
+def warn_swift_newtype_attribute_non_typedef
+  : Warning<"%0 attribute may be put on a typedef only; attribute is ignored">,
+    InGroup<DiagGroup<"swift-newtype-attribute">>;
----------------
aaron.ballman wrote:
> Hrm, we already have existing diagnostics to cover this (`warn_attribute_wrong_decl_type_str`) but it's in the `IgnoredAttributes` group. Do you need a new warning group specific to this? Is there a reason that group is not a subset of `IgnoredAttributes`?
Hmm, Im okay with changing the group, but I do wonder if Apple is going to worry about diagnostic compatibility.  @doug.gregor, @rjmccall, or @dexonsmith would be better suited to answer the question about command line compatibility.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87652/new/

https://reviews.llvm.org/D87652