[cfe-commits] r59884 - in /cfe/trunk: include/clang/AST/Expr.h lib/AST/ExprConstant.cpp lib/Sema/SemaStmt.cpp test/Sema/switch.c

[Daniel Dunbar]

Hi Anders,

Some comments,

On Sat, Nov 22, 2008 at 1:50 PM, Anders Carlsson <andersca at mac.com> wrote:
> +  // FIXME: We should come up with a better API for the isEvaluated case.
> +  bool Evaluate(APValue& Result, ASTContext &Ctx, bool *isEvaluated = 0) const;
> +

I agree with the FIXME. :)

I personally think that Evaluate should return a simple variant class
which wraps all the information Evaluate computed about the
expression. This forces clients to explicitly deal with the variant
which increases code size (lines) a bit but I think for this case this
is Good Thing.

This name is quite confusing (esp. given the addition of the
isEvaluatable method which computes something else) and is not, I
think, what we want to know. For the sole current use case, what we
really want to know is 'must-be-executed', where false means that
replacing the expression by the evaluated constant does not change the
semantics of the program. The current flag is, I believe, just a
coarse approximation of this. It is safe to replace (0,1) by 1, for
example.

>   /// isEvaluatable - Call Evaluate to see if this expression can be constant
>   /// folded, but discard the result.
>   bool isEvaluatable(ASTContext &Ctx) const;

This can be inline? I can't say I'm fond of the name evaluatable
because I don't know what it means, but in this case I presume it
"means" Evaluate returns true for the expression.

 - Daniel