[cfe-commits] r59420 - /cfe/trunk/lib/AST/ExprConstant.cpp
Anders Carlsson
andersca at mac.com
Sun Nov 16 14:45:50 PST 2008
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>
More information about the cfe-commits
mailing list