[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",
----------------
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?)


================
Comment at: clang/include/clang/Basic/Attr.td:2183
+  let Subjects = SubjectList<[TypedefName], ErrorDiag>;
+  let Documentation = [SwiftNewTypeDocs];
+}
----------------
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?


================
Comment at: clang/include/clang/Basic/AttrDocs.td:3585
+ypedef.
+  }];
+}
----------------
Code examples of how to use this properly would be appreciated.


================
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">>;
----------------
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`?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5894
+  // Make sure that there is an identifier as the annotation's single argument.
+  if (!checkAttributeNumArgs(S, AL, 1)) {
+    AL.setInvalid();
----------------
Btw, if you fix up the parser, then you can skip setting `HasCustomParsing` and then can remove some of this manual error checking as well.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5895
+  if (!checkAttributeNumArgs(S, AL, 1)) {
+    AL.setInvalid();
+    return;
----------------
No need to do this (I'm surprised it even compiles given that `AL` is a `const` reference), here or elsewhere.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5905
+
+  IdentifierInfo *II = AL.getArgAsIdent(0)->Ident;
+  SwiftNewTypeAttr::NewtypeKind Kind;
----------------
There should be a helper method to do this dance for you. `SwiftNewTypeAttr::ConvertStrToNewtypeKind()` should be roughly what it's called (it's generated for you by the attr emitter).


================
Comment at: clang/test/SemaObjC/attr-swift-newtype.m:2
+// RUN: %clang_cc1 -verify -fsyntax-only %s
+// RUN: not %clang_cc1 -ast-dump %s | FileCheck %s
+
----------------
Can you split the AST bits out into their own test in the AST directory?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87652