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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
