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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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",
----------------
compnerd wrote:
> 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`.
Ah, thank you for the explanation. I think we should document `swift_wrapper` as explicitly being deprecated then so it's clear to people (esp Future Aaron) what the intent is. It sounds like there's no need for `AdditionalMembers` (that's only useful if you need to distinguish between the spellings that share a semantic attribute).


================
Comment at: clang/include/clang/Basic/Attr.td:2183
+  let Subjects = SubjectList<[TypedefName], ErrorDiag>;
+  let Documentation = [SwiftNewTypeDocs];
+}
----------------
compnerd wrote:
> 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.
If you can easily modify `ParseAttributeArgsCommon`, then awesome! If that turns out to be a slog, I don't insist on the change being made.


================
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">>;
----------------
compnerd wrote:
> 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.
If you need specific control over the swift attribute diagnostics for ignored attributes, you could make the group be a part of `IgnoredAttributes` (so by default, swift ignored attributes are controlled by `IgnoredAttributes` but can be selectively enabled/disabled).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87652