[cfe-commits] [PATCH] Use the new APFloat::convertToInt(APSInt) function (issue4728042)

[Jeffrey Yasskin]

On Thu, Jul 14, 2011 at 6:14 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
> On Thu, Jul 14, 2011 at 6:06 PM, Jeffrey Yasskin <jyasskin at google.com> wrote:
>> On Thu, Jul 14, 2011 at 5:30 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
>>> On Thu, Jul 14, 2011 at 5:06 PM,  <jyasskin at gmail.com> 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.