[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

Alright, LGTM.



================
Comment at: clang/include/clang/AST/DeclCXX.h:1013
   CXXMethodDecl *getLambdaStaticInvoker() const;
+  CXXMethodDecl *getLambdaStaticInvoker(CallingConv CC) const;
+
----------------
erichkeane wrote:
> erichkeane wrote:
> > rjmccall wrote:
> > > Probably worth clarifying in the comment which invoker is returned by the no-arguments variant.
> > I just looked and the first is actually only used 1x!  I think I can just replace that usage and remove the former overload if that is acceptable to you.
> On second thought... I might leave this alone.  Otherwise the ItaniumMangler basically needs to reproduce the functionality.  I think the improved comment is an appropriate change here.
Fine by me.


================
Comment at: clang/lib/AST/DeclCXX.cpp:1550
+                 return FTy->getCallConv() == CC;
+               });
+
----------------
erichkeane wrote:
> rjmccall wrote:
> > This seems both rather more complicated and rather more expensive than a simple loop which assigns to a local pointer.
> Do you mean the filtering copy-if?  The idea was to make it so that the assert below still applies.  If that isn't necessary, this DOES simplify to a std::find and a return.
I mean that if you really want to assert that, you could do so by checking whether the existing function is the same as what you've already found; you don't need a vector.  But just not checking that seems fine to me.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89559/new/

https://reviews.llvm.org/D89559