[PATCH] D87534: Sema: introduce `__attribute__((__swift_name__))`

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:2174
+                   ObjCInterface, ObjCClassMethod, ObjCInstanceMethod, ObjCProperty, ObjCProtocol],
+      ErrorDiag, "ExpectedSwiftNameSubjects">;
+  let Documentation = [SwiftNameDocs];
----------------
I don't see anything adding `ExpectedSwiftNameSubjects` to the list of diagnostic enumerations, are you sure this is needed?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3985
+def warn_attr_swift_name_basename_invalid_identifier
+  : Warning<"%0 attribute has invalid identifier for base name">,
+    InGroup<SwiftNameAttribute>;
----------------
I think several of these can be consolidated into a single diagnostic with `%select`:

`%0 attribute has invalid identifier for %select{context name|base name|parameter name}1`


================
Comment at: clang/include/clang/Sema/Sema.h:1839
+  /// attribute for the decl \p D. Raise a diagnostic if the name is invalid
+  /// for the given declaration.
+  ///
----------------
The docs should probably mention what `AL` is used for.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4282
+                                        StringRef Name, bool Override) {
+  if (const SwiftNameAttr *Inline = D->getAttr<SwiftNameAttr>()) {
+    if (Override) {
----------------
`const auto *`


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4289
+    if (Inline->getName() != Name && !Inline->isImplicit()) {
+      Diag(Inline->getLocation(), diag::warn_attribute_ignored) << Inline;
+      Diag(CI.getLoc(), diag::note_conflicting_attribute);
----------------
I think it would be more helpful if the diagnostic said why the attribute is being ignored (because the arguments don't match).


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5658
+static Optional<unsigned>
+validateSwiftFunctionName(StringRef Name, unsigned &SwiftParamCount,
+                          bool &IsSingleParamInit) {
----------------
FWIW, I'm not particularly qualified to review the logic here. Someone with more Swift experience should look at this bit.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5660
+                          bool &IsSingleParamInit) {
+  SwiftParamCount = 0;
+
----------------
Set `IsSingleParamInit` as well (nothing sets it before the early return on line 5673)?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5804-5805
+    if (const auto *Method = dyn_cast<ObjCMethodDecl>(D)) {
+      ParamCount = Method->getSelector().getNumArgs();
+      Params = Method->parameters().slice(0, ParamCount);
+    } else {
----------------
Do you have to worry about functions with `...` variadic parameters and how those impact counts (for either ObjC or regular methods)?


================
Comment at: clang/test/SemaObjC/attr-swift-name.m:1
+// RUN: %clang_cc1 -verify -fsyntax-only -fobjc-arc -fblocks %s
+
----------------
The `fblocks` makes me wonder -- should this attribute be one you can write on a block?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87534