[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added inline comments.


================
Comment at: clang/lib/AST/Expr.cpp:1949-1950
 
     if (E->getCastKind() == CK_ConstructorConversion)
       return cast<CXXConstructExpr>(SubExpr)->getConstructor();
 
----------------
kimgr wrote:
> davrec wrote:
> > I think the errors prove we should fall back to the most conservative possible fix: remove all the other changes and just change these 2 lines to:
> > ```
> > if (E->getCastKind() == CK_ConstructorConversion) {
> >   if (auto *CE = dyn_cast<ConstantExpr>(SubExpr)
> >     SubExpr = CE->getSubExpr();
> >   return cast<CXXConstructExpr>(SubExpr)->getConstructor();
> > }
> > ```
> > I'm can't quite remember but I believe that would solve the bug as narrowly as possible.  @kimgr can you give it a try if and see if it avoids the errors?
> > (If it doesn't, I'm stumped...)
> Yes, it does. I needed to add the same `ConstantExpr` skipping to the user-defined conversion handling below, but once those two were in place the new unittests succeed and the existing lit tests also.
> 
> It does feel a little awkward, though... I wish I had a clearer mental model of how the implicit-skipping is supposed to work. My intuition is telling me `skipImplicitTemporary` should probably disappear and we should use the `IgnoreExpr.h` facilities directly instead, but it's all a little fuzzy to me at this point.
> 
> I'll run the full `check-clang` suite overnight with this alternative patch, I have no reason to think there will be problems, but I'll make sure in the morning.
> 
> Thanks!
I can confirm that full `check-clang` also works with the narrower fix. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117391