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