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

[Neil Booth]

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.

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

Neil.