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

[Anders Carlsson]

16 nov 2008 kl. 13.59 skrev Eli Friedman:

> On Sun, Nov 16, 2008 at 11:01 AM, Anders Carlsson <andersca at mac.com>  
> wrote:
>> Author: andersca
>> Date: Sun Nov 16 13:01:22 2008
>> New Revision: 59420
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=59420&view=rev
>> Log:
>> More work on the constant evaluator. Eli, it would be great if you  
>> could have a look at this.
>
> Okay... it looks generally fine.  More comments below:
>
>> +APValue  
>> LValueExprEvaluator::VisitArraySubscriptExpr(ArraySubscriptExpr *E)
>> +{
>> +  APValue Result;
>> +
>> +  if (!EvaluatePointer(E->getBase(), Result, Info))
>> +    return APValue();
>
> I don't think this can actually happen, but it might be a good idea to
> double-check that the base is in fact a pointer.
>

Hmm, not sure what you mean. EvaluatePointer will return false if the  
base is not a pointer.

>> +  APSInt Index;
>> +  if (!EvaluateInteger(E->getIdx(), Index, Info))
>> +    return APValue();
>> +
>> +  uint64_t ElementSize = Info.Ctx.getTypeSize(E->getType()) / 8;
>> +
>> +  uint64_t Offset = Index.getSExtValue() * ElementSize;
>
> This could potentially crash once we support integers larger than 64
> bits.  Also, this needs to be aware of the sign; we don't want to
> sign-extend an unsigned short.

I tried many different examples and couldn't come up with one that  
would fail. Do you have a concrete example? :)

>
>
>> +  bool VisitCXXBoolLiteralExpr(const CXXBoolLiteralExpr *E) {
>> +    Result = E->getValue();
>> +    Result.setIsUnsigned(E->getType()->isUnsignedIntegerType());
>> +    return true;
>> +  }
>
> I think it would be a good idea to set the width of the result  
> explicitly.
>

Good idea. Fixed.

>> +  if (E->getOpcode() == BinaryOperator::Sub) {
>> +    if (LHSTy->isPointerType()) {
>> +      if (RHSTy->isIntegralType()) {
>> +        // pointer - int.
>> +        // FIXME: Implement.
>
> It should be impossible to land here for that case: pointer-int
> doesn't return an int.
>

Fixed!

>> +      // FIXME: Is this correct? What if only one of the operands  
>> has a base?
>> +      if (LHSValue.getLValueBase() || RHSValue.getLValueBase())
>> +        return false;
>
> This is conservatively correct; a more aggressive constraint would be
> that the bases are identical.
>

Hmm, OK.

>> +      const QualType Type = E->getLHS()->getType();
>> +      const QualType ElementType = Type->getAsPointerType()- 
>> >getPointeeType();
>> +
>> +      uint64_t D = LHSValue.getLValueOffset() -  
>> RHSValue.getLValueOffset();
>> +      D /= Info.Ctx.getTypeSize(ElementType) / 8;
>
> The result here isn't necessarily positive; an unsigned divide will
> give an incorrect result in such cases.
>

Same here. I couldn't reproduce the error with an example.

>> +      Result = D;
>> +      Result.zextOrTrunc(getIntTypeSizeInBits(E->getType()));
>> +      Result.setIsUnsigned(E->getType()->isUnsignedIntegerType());
>
> You need to set the width of the result first; otherwise, you might
> incorrectly zext the result.
>

Fixed!

Thanks for the review Eli!

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/20081116/10bf62ff/attachment.bin>