[PATCH] D76396: Allow immediate invocation of constructors

[Wyatt Childers via Phabricator via cfe-commits]

wchilders marked an inline comment as done.
wchilders added inline comments.


================
Comment at: clang/lib/Sema/SemaInit.cpp:6466
     if (Entity.allowsNRVO())
       CurInit = S.BuildCXXConstructExpr(Loc, Step.Type,
                                         Step.Function.FoundDecl,
----------------
wchilders wrote:
> rsmith wrote:
> > It looks like the other callers to `BuildCXXConstructExpr` are also missing this handling. Can we put the call to `CheckForImmediateInvocation` in `BuildCXXConstructExpr` to handle all those cases at once, or do we need to defer it until after the other stuff below?
> Those changes shouldn't have impact here. 
> 
> I choose to apply this here as when looking at other uses of `CheckForImmediateInvocation`, `MaybeBindToTemporary` was performed on the expr, prior to the expr being passed to `CheckForImmediateInvocation`.
> 
> Upon changing this over, all tests (upstream and ours) still pass, so if you don't see any issues, I'm fine with moving this into `BuildCXXConstructExpr`. Additionally, upon closer inspection, there is low coupling between `MaybeBindToTemporary` and `CXXConstructExpr`, where as there is significant coupling for ObjC and `CallExpr`s. Unless something I've said here gives you some doubts, I'm fine with moving this to `BuildCXXConstructExpr`, there's nothing that immediately jumps out to me as a problem.
Upon changing this over, all tests (upstream and ours) still pass~~, so if you don't see any issues, I'm fine with moving this into BuildCXXConstructExpr~~.  (Whoops, rearranged some sentences last second heh)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76396