[PATCH] D87534: Sema: introduce `__attribute__((__swift_name__))`
Saleem Abdulrasool via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 16 12:02:51 PDT 2020
compnerd marked 3 inline comments as done.
compnerd added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:104
+def ObjCClassMethod
+ : SubsetSubject<ObjCMethod, [{!S->isInstanceMethod()}],
----------------
aaron.ballman wrote:
> This change is no longer needed.
Ah, right, I'll move it to the `swift_private` change.
================
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);
----------------
aaron.ballman wrote:
> 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).
Okay, I don't think that there is an existing warning, but I should be able to add one.
================
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 {
----------------
aaron.ballman wrote:
> 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()`?)
I'll add a test case demonstrating that, it is indeed missing.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87534/new/
https://reviews.llvm.org/D87534
More information about the cfe-commits
mailing list