[PATCH] D103395: PR45879: Keep evaluated expression in LValue object

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

Thank you for feedback!

In D103395#3096177 <https://reviews.llvm.org/D103395#3096177>, @tambre wrote:

> This breaks the following code:
>
>   struct sub
>   {
>   	char data;
>   };
>   
>   struct main
>   {
>   	constexpr main()
>   	{
>   		member = {};
>   	}
>   
>   	sub member;
>   };
>   
>   constexpr main a{};
>
> With:
>
>   fmt.cpp:16:16: error: constexpr variable 'a' must be initialized by a constant expression
>   constexpr main a{};
>                  ^~~
>   1 error generated.
>
> Clang trunk and GCC (Debian 11.2.0-10) handle it fine.
> Noticed in libfmt <https://github.com/fmtlib/fmt/issues/2571#issuecomment-953887675>.

Strange, I see that it cannot be compiled neither by gcc nor by clang: https://godbolt.org/z/1dY9Gs6zM. Do I miss something?



================
Comment at: clang/lib/AST/ExprConstant.cpp:1584-1585
       Designator = SubobjectDesignator(getType(B));
+      if (!LExpr)
+        LExpr = B.dyn_cast<const Expr *>();
       IsNullPtr = false;
----------------
aaron.ballman wrote:
> Should we be asserting that `LExpr` is null?
`LExpr` is initialized only once, when LValue starts evaluation. Later on the evaluation can proceed to subexpressions but the upper expression is sufficient to detect active union member change.


================
Comment at: clang/lib/AST/ExprConstant.cpp:8049
 
+  bool evaluate(const Expr *E) {
+    Result.LExpr = E;
----------------
aaron.ballman wrote:
> It's not super clear to me when consumers should call `Evaluate()` instead of calling `Visit()`. Some comments on the function may help make it more clear.
`Visit` methods are now made non-public, hopefully it can make usage more clear.


================
Comment at: clang/lib/AST/ExprConstant.cpp:8050
+  bool evaluate(const Expr *E) {
+    Result.LExpr = E;
+    return Visit(E);
----------------
aaron.ballman wrote:
> Should we be asserting that `Result.LExpr` is not already set?
`Result.LExpr` is updated while evaluator descend the evaluated tree, so it is normal to see it set. 


================
Comment at: clang/lib/AST/ExprConstant.cpp:8586
 
+  bool evaluate(const Expr *E) { return Visit(E); }
+
----------------
aaron.ballman wrote:
> Is there a reason we're not setting `Result.LExpr` here?
Actually This method was used in the initial implementation, now it can be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103395