ahatanak accepted this revision. ahatanak added a comment. This revision is now accepted and ready to land.
LGTM ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3392 + // replace instancetype with the class type + if (Ty.getTypePtr() == S.Context.getObjCInstanceTypeDecl()->getTypeForDecl()) + if (auto *OMD = dyn_cast<ObjCMethodDecl>(D)) ---------------- You can use `ASTContext::getObjCInstanceType`, which returns `QualType`. ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3403 } Ty = getFunctionOrMethodResultType(D); if (!isNSStringType(Ty, S.Context, /*AllowNSAttributedString=*/true) && ---------------- fcloutier wrote: > ahatanak wrote: > > Is it possible to just replace `Ty` with the class pointer type here if it > > is an instancetype instead of defining another function > > (`isNSStringInterface`) that looks at the class name and determines whether > > it's `NSString`? > I think that it would be awkward and I'd like to offer modest pushback. You > need the enclosing `Decl` to make sense of `instancetype` because it's just a > typedef to `id`. There are three uses of `isNSStringType` and only one could > benefit from the `Decl` it because the other two don't refer to a return > type. IMO, the split is the right way to go. > > FWIW, it's not so much defining another function that looks at the class name > so much as moving the look-at-the-class-name logic out of an existing > function. I took the guts out of `isNSStringType` to make > `isNSStringInterface`, but Phabricator isn't very good at showing code that > moved. Oh I see. But I still feel this is better since `isNSStringInterface` would be called twice in the previous patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112670/new/ https://reviews.llvm.org/D112670 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
