[llvm-commits] [llvm] r51132 - /llvm/trunk/lib/Support/APFloat.cpp

[Evan Cheng]

On May 14, 2008, at 3:42 PM, Neil Booth wrote:

> Neil Booth wrote:-
>
>> Dale Johannesen wrote:-
>>
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> ====================================================================
>>> --- llvm/trunk/lib/Support/APFloat.cpp (original)
>>> +++ llvm/trunk/lib/Support/APFloat.cpp Wed May 14 17:05:31 2008
>>> @@ -2003,7 +2003,7 @@
>>>   firstSignificantDigit = p;
>>>
>>>   for(;;) {
>>> -    unsigned int hex_value;
>>> +    uint64_t hex_value;
>>>
>>>     if(*p == '.') {
>>>       assert(dot == 0);
>>
>> Looks good (the left shift being the problem, right?).
>
> Actually Dale this is still not right; there is nothing stopping an
> integerPart being 96 or 128 bits so uint64_t is incorrect.  The code
> as I originally wrote it is type-width neutral.
>
> The line should be reverted to as it was before the below patch.
>
> Evan, your patch
>
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/APFloat.cpp?r1=49983&r2=50590&diff_format=h
>
> didn't add much clarity.  I coded APFloat carefully and don't believe
> your patch fixed a real-world bug, but would be happy to be proven
> wrong.  However your patch has introduced at least one bug that wasn't
> originally present, found by Dale here.

Well, sorry about introducing a bug. Obviously the patch isn't  
intended to fix a bug. It's meant to suppress compile time warnings  
(with -m64 -Wshorten-64-to-32). Unfortunately it's a strict  
requirement for some.

>
>
> Could you review carefully every line of your patch to see if it
> hasn't introduced others, or consider reverting it?

If you think the patch is dangerous. Please go ahead and revert  
APFloat.cpp portion of the patch.

Evan

>
>
> Neil.
>