[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();
 
----------------
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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117391