[PATCH] D105756: [clang] C++98 implicit moves are back with a vengeance

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D105756#3001045 <https://reviews.llvm.org/D105756#3001045>, @NoQ wrote:

> I guess I'm confused about the exact meaning of the AST. I'm not sure whether it's actively incorrect or just changed meaning. I'll definitely be debugging this further.
>
> My backstory here is that the static analyzer can be thought of as (primitivized down to) an AST "interpreter". It arranges AST nodes in the order in which they're "executed" at run-time (aka "Clang CFG") and then processes them one node at a time in that order - unlike CodeGen that processes the nodes more or less recursively. This causes us static analyzer people to have an exotic taste for the good AST as we're required to understand every node literally in isolation from other nodes. Every time the nodes make sense together/recursively but not individually our life becomes much harder.
>
> So in the new AST (which wouldn't exist with your fix so we may be talking about vacuous truths here, as I'm still to look at the C++ variant of the same AST) I'm confused about the seemingly brand-new AST node that you introduced, `ImplicitCastExpr xvalue <NoOp>` - a cast that changes value kind from prvalue to glvalue and in this regard appears to behave exactly like `MaterializeTemporaryExpr` - which was hard enough to support the first time (due to its deep recursive nature where it extracts the storage address that was already "lost" in the operand), I really don't want to do it again :) And I'm also confused by the situation where `ImplicitCastExpr <LValueToRValue>` does not correspond to a memory load like it typically does.

The xvalue cast used in NRVO should never apply to a prvalue, as they are used only in expressions that refer to a variable / parameter, which are always lvalues, so it's really only changing categories while still staying within glvalues.

The NoOp cast, as far as code generation / execution is concerned, does nothing really. The purpose of a NoOp cast which changes value category lies in semantic analysis, where it will help things like deciding what kind overload will be selected or what kinds of references an expression can be bound to. Seeing those casts in the middle of a complete AST can only help make sense of why certain overloads where selected, but it would not aid in interpreting how the program should be executed.

> If I understand correctly, in C++ the innermost lvalue-to-rvalue cast wouldn't exist, therefore the double memory load concern will be eliminated. I.e., it'll probably be something like

As far as the clang CodeGen is concerned, there is no double memory load here, when starting from 
the left in the "LValueToRValue -> NoOp -> LValueToRValue" sequence, the code that implements this in CGExpr EmitLValue function will just recursively eat the NoOp cast (EmitCastLValue) and then go back where it started recursively calling EmitLValue again, making this equivalent to just a single `LValueToRValue`, as far as what code is actually emitted.

>   ReturnStmt
>   `- ImplicitCastExpr <LValueToRValue>
>      `- ImplicitCastExpr <NoOp> xvalue  // MaterializeTemporaryExpr xvalue?
>         `- CXXConstructExpr

I think that is not correct, in C++ when dealing with classes, the CXXConstructExpr would stand for the LValueToRValue cast there.
The implicit cast to xvalue would also stand between the DeclRef and the CXXConstruct / LValueToRValue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105756