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