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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
