[cfe-commits] r65524 - /cfe/trunk/lib/AST/Expr.cpp

[Daniel Dunbar]

Very nice; I like this approach of keeping the isICE determination
very localized while still leveraging Evaluate.

Two comments:

On Thu, Feb 26, 2009 at 1:29 AM, Eli Friedman <eli.friedman at gmail.com> wrote:
> -bool Expr::isIntegerConstantExpr(llvm::APSInt &Result, ASTContext &Ctx,
> -                                 SourceLocation *Loc, bool isEvaluated) const {
> -  if (!isIntegerConstantExprInternal(Result, Ctx, Loc, isEvaluated))
> -    return false;
> -  assert(Result == EvaluateAsInt(Ctx) && "Inconsistent Evaluate() result!");

I think it would be nice to continue to assert that we can Evaluate
anything that isICE recognizes. It should be much more obvious that
this holds now, but its still nice to enforce the invariant.

> +// CheckICE - This function does the fundamental ICE checking: the returned
> +// ICEDiag contains a Val of 0, 1, or 2, and a possibly null SourceLocation.
> +// Note that to reduce code duplication, this helper does no evaluation
> +// itself; the caller checks whether the expression is evaluatable, and
> +// in the rare cases where CheckICE actually cares about the evaluated
> +// value, it calls into Evalute.
> +//
> +// Meanings of Val:
> +// 0: This expression is an ICE if it can be evaluated by Evaluate.
> +// 1: This expression is not an ICE, but if it isn't evaluated, it's
> +//    a legal subexpression for an ICE. This return value is used to handle
> +//    the comma operator in C99 mode.
> +// 2: This expression is not an ICE, and is not a legal subexpression for one.

Enumify? I think it improves the readability of the code below. In
fact, with an operator< & std::max it may simplify returning the
correct ICEDiag as well.

 - Daniel