rsmith added a comment. In D62009#1506415 <https://reviews.llvm.org/D62009#1506415>, @Tyker wrote:
> there are issues with using ConstantExpr to mark expressions as constant for > semantic checking: > > - #1 multpile Expr::Ignore* operation remove ConstantExpr from the expression. `Ignore*` is generally used when walking the syntactic form of an expression, so this is usually appropriate. > - #2 Semantic checking Traverses the AST so all methods that only mark the > top-level Node will not completely work. Semantic checks that care about constant evaluation should not walk over a `ConstantExpr` node. Generally, we want rather different checking inside a `ConstantExpr` node than outside (for example, we shouldn't be performing any checks for potential overflow or narrowing or conversions, and instead should be finding out what *actually* happens by evaluating the expression and diagnosing problems in it). So this seems fine too, though there may still be places that need updates to properly handle `ConstantExpr`. > - #3 at short term: adding ConstantExpr modifies the AST structure, so adding > it everywhere it is needed for semantic checking will require changing code > that depend closely on the AST structure. Yes. But that's going to be the case for any approach we take here. > - push a ExpressionEvaluationContext::ConstantEvaluated so Sema will > propagate it and propagate from Sema to the expression evaluator via boolean > values. > - put all semantic checking function's in a SemaCheckContext class and > propagate via this class to the expression evaluator. this class will be > usable to propagate Sema and probably other informations. > - keep the bit in ExprBitfield but make all nodes created in > ExpressionEvaluationContext::ConstantEvaluated marked for constant evaluation > to fixes limitation #2. We don't always know whether an expression is constant evaluated until after we've parsed it (eg, for a call to a function that might be `consteval` we need to perform overload resolution before we find out). And this would require changing all the (many) places that create `Expr` nodes to pass in new information. That sounds unpleasant and expensive, and adds a permanent cost to all future AST changes. So far we don't appear to need any per-`Expr` indicator of whether that expression is transitively within a `ConstantExpr`, so let's not add that. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62009/new/ https://reviews.llvm.org/D62009 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
