[PATCH] D112670: __attribute__((format_arg(__NSString__, N))) does not support instancetype in NSString interface

[Akira Hatanaka via Phabricator via cfe-commits]

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