On Thu, Jul 14, 2011 at 6:14 PM, Eli Friedman <[email protected]> wrote: > On Thu, Jul 14, 2011 at 6:06 PM, Jeffrey Yasskin <[email protected]> wrote: >> On Thu, Jul 14, 2011 at 5:30 PM, Eli Friedman <[email protected]> wrote: >>> On Thu, Jul 14, 2011 at 5:06 PM, <[email protected]> wrote: >>>> Reviewers: cfe-commits_cs.uiuc.edu, >>>> >>>> Message: >>>> Initial diff at >>>> http://codereview.appspot.com/download/issue4728042_1.diff. >>>> >>>> Description: >>>> Use the new APFloat::convertToInt(APSInt) function to simplify uses of >>>> convertToInt(integerParts*) and make them more reliable. >>>> >>>> Relies on http://codereview.appspot.com/4738043. >>>> >>>> Please review this at http://codereview.appspot.com/4728042/ >>>> >>>> Affected files: >>>> M lib/AST/ExprConstant.cpp >>>> M lib/Sema/SemaChecking.cpp >>>> >>>> >>>> Index: lib/AST/ExprConstant.cpp >>>> diff --git a/lib/AST/ExprConstant.cpp b/lib/AST/ExprConstant.cpp >>>> index >>>> 9943222b1dd30d9761731fca984d20d6b578f788..03c0747efba9e8ed9451607b6a33c8b1c9716196 >>>> 100644 >>>> --- a/lib/AST/ExprConstant.cpp >>>> +++ b/lib/AST/ExprConstant.cpp >>>> @@ -223,12 +223,10 @@ static APSInt HandleFloatToIntCast(QualType DestType, >>>> QualType SrcType, >>>> // Determine whether we are converting to unsigned or signed. >>>> bool DestSigned = DestType->isSignedIntegerOrEnumerationType(); >>>> >>>> - // FIXME: Warning for overflow. >>>> - uint64_t Space[4]; >>>> + APSInt Result(DestWidth, !DestSigned); >>>> bool ignored; >>>> - (void)Value.convertToInteger(Space, DestWidth, DestSigned, >>>> - llvm::APFloat::rmTowardZero, &ignored); >>>> - return APSInt(llvm::APInt(DestWidth, 4, Space), !DestSigned); >>>> + (void)Value.convertToInteger(Result, llvm::APFloat::rmTowardZero, >>>> &ignored); >>>> + return Result; >>>> } >>> >>> Why are you removing the "FIXME: Warning for overflow" comment? The >>> issue still exists... >> >> Oops. I thought that was talking about overflowing the 4 slots in >> Space. I've put it back. >> >>>> static APFloat HandleFloatToFloatCast(QualType DestType, QualType SrcType, >>>> Index: lib/Sema/SemaChecking.cpp >>>> diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp >>>> index >>>> 267cda8f4016ca935873eac6d22bb82906201051..d1990f3061f353f3152a33d450b971b445d95a10 >>>> 100644 >>>> --- a/lib/Sema/SemaChecking.cpp >>>> +++ b/lib/Sema/SemaChecking.cpp >>>> @@ -2990,17 +2990,15 @@ void DiagnoseFloatingLiteralImpCast(Sema &S, >>>> FloatingLiteral *FL, QualType T, >>>> if (&Value.getSemantics() == &llvm::APFloat::PPCDoubleDouble) >>>> return; >>>> >>>> - // Try to convert this exactly to an 64-bit integer. FIXME: It would be >>>> - // nice to support arbitrarily large integers here. >>>> + // Try to convert this exactly to an integer. >>>> bool isExact = false; >>>> - uint64_t IntegerPart; >>>> - if (Value.convertToInteger(&IntegerPart, 64, /*isSigned=*/true, >>>> + llvm::APSInt IntegerValue(S.Context.getIntWidth(T), >>>> + /*isUnsigned=*/false); >>>> + if (Value.convertToInteger(IntegerValue, >>>> llvm::APFloat::rmTowardZero, &isExact) >>>> != llvm::APFloat::opOK || !isExact) >>>> return; >>>> >>>> - llvm::APInt IntegerValue(64, IntegerPart, /*isSigned=*/true); >>>> - >>>> std::string LiteralValue = IntegerValue.toString(10, /*isSigned=*/true); >>>> S.Diag(FL->getExprLoc(), diag::note_fix_integral_float_as_integer) >>>> << FixItHint::CreateReplacement(FL->getSourceRange(), LiteralValue); >>> >>> I was going to ask why you're forcing this to be signed, but then I >>> realized it's impossible to write a negative floating-point literal. >>> :) >> >> More because the previously-existing code made it signed. I didn't >> think hard enough to notice that negative floating literals are >> impossible. Using signed integers may actually be a bug, since if the >> target is a uint64, and the floating literal doesn't fit in an int64, >> this will fail the conversion even though the result would fit. > > Hmm... might actually be a regression for something like "unsigned > short x = 60000.;" ? Should be trivial to call isSignedIntegerType() > here, though.
Done, with hasUnsignedIntegerRepresentation because I'm not totally convinced vectors aren't sneaking into here. > Otherwise looks fine. Thanks, committed as r135279. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
