[cfe-commits] r53221 - /cfe/trunk/lib/AST/ExprConstant.cpp

[Eli Friedman]

On Mon, Jul 7, 2008 at 10:49 PM, Anders Carlsson <andersca at mac.com> wrote:
> +  APValue VisitBinaryOperator(const BinaryOperator *E) {
> +    // The LHS of a constant expr is always evaluated and needed.
> +    llvm::APSInt Result(32);
> +    if (!Evaluate(E->getRHS(), Result, Ctx))
> +      return APValue();
> +
> +    llvm::APSInt RHS(32);
> +    if (!Evaluate(E->getRHS(), RHS, Ctx))
> +      return APValue();

Typo with getRHS twice?  Also, we don't need/want to evaluate the RHS
for && and ||.

> +    case BinaryOperator::Shl:
> +      Result <<=
> +        static_cast<uint32_t>(RHS.getLimitedValue(Result.getBitWidth()-1));
> +      break;
> +    case BinaryOperator::Shr:
> +      Result >>=
> +        static_cast<uint32_t>(RHS.getLimitedValue(Result.getBitWidth()-1));
> +      break;

Maybe we should fail if the RHS is larger than the bitwidth of the
Result?  It's undefined behavior, so attempting to give a result seems
confusing.

> +    case BinaryOperator::Comma:
> +      // C99 6.6p3: "shall not contain assignment, ..., or comma operators,
> +      // *except* when they are contained within a subexpression that is not
> +      // evaluated".  Note that Assignment can never happen due to constraints
> +      // on the LHS subexpr, so we don't need to check it here.
> +      // FIXME: Need to come up with an efficient way to deal with the C99
> +      // rules on evaluation while still evaluating this.  Maybe a
> +      // "evaluated comma" out parameter?
> +      return APValue();
> +    }

Mmmm... actually, thinking it over a bit more, we don't actually need
to do anything here unless we care about evaluating comma operators.
I can't imagine any situation where that would be useful, since C
constant expressions aren't allowed to contain evaluated commas, and
nobody would write real code with a comma with both sides constant.

> +      // sizeof(vla) is not a constantexpr: C99 6.5.3.4p2.
> +      if (!E->getSubExpr()->getType()->isConstantSizeType()) {
> +        // FIXME: Should we attempt to evaluate this?

We definitely should evaluate the alignof case.  For the sizeof case,
I think it's also reasonable to try and evaluate it.  (Although doing
that does run into an O(n^2) issue with many-dimensional VLAs... not
sure what to do about that.  I'm almost tempted to cache constant
expression evaluation results, but it would be a lot of memory bloat.)

-Eli