aaron.ballman added inline comments.

================
Comment at: clang/include/clang/Basic/Attr.td:104
 
+def ObjCClassMethod
+    : SubsetSubject<ObjCMethod, [{!S->isInstanceMethod()}],
----------------
This change is no longer needed.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:3584
+The name may be a compound Swift name.  For a type, enum constant, property, or
+variable declaration, the name must be a simple or qualified identifier.
+  }];
----------------
A code example showing proper usage would be appreciated.


================
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);
----------------
compnerd wrote:
> aaron.ballman wrote:
> > I think it would be more helpful if the diagnostic said why the attribute 
> > is being ignored (because the arguments don't match).
> Does the note below not accomplish that?
Not really, no. The warning diagnostic itself just says that the attribute is 
ignored, which is the effect but not the rationale. The note (which is easier 
for folks to ignore) says the attribute is conflicting, but conflicting with 
*what* (there could be a half dozen attributes on the same declaration, for 
instance).


================
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 {
----------------
compnerd wrote:
> aaron.ballman wrote:
> > Do you have to worry about functions with `...` variadic parameters and how 
> > those impact counts (for either ObjC or regular methods)?
> No, they are currently not auto-imported AFAIK.
Should such function signatures be rejected here though? (If so, can you add a 
test for that case as well as a test using a K&R C declaration like `void f()`?)


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

Reply via email to