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

[Anders Carlsson]

24 nov 2008 kl. 15.18 skrev 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.
>

Another option would be to pass in the constraints to Evaluate, like  
"this must be fully evaluated", "this must be an ICE" etc.

> 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.
>

Yeah - canEvaluate would be better - or just have an Evaluate overload  
that wouldn't take an APValue reference.

Anders
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 2415 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20081124/6affaffc/attachment.bin>