1
\$\begingroup\$

This is a follow up to https://codereview.stackexchange.com/questions/272333/conversion-from-long-double-to-string where I do the reverse conversion. This time around it is string to long double. It parses up to 38 dignificant digits, and I used a 128 significant bit floating point multiplication to convert decimal to binary, including the use of FE_TOWARDZERO mode to salvage the high significant bits from long double multiplications.

This code is designed for compilers with a long double of 64 significant bits (like Intel extended precision).

#include <stdint.h>
#include <fenv.h>
#include <string>

struct binary128tmp{
 uint64_t m[2]; // 64 bits in each integer
 short e;
};

struct int128tmp{
 uint64_t q[2];

 operator uint64_t(void){
  return q[0];
 }

 operator long double(void){
  return q[1]*0x1.p64l+q[0];
 }
};

 int128tmp toint128tmp(uint64_t a){
  return {a, 0};
 }

int128tmp operator*(int128tmp z, int128tmp x){
// rounding mode toward zero at this point
    int128tmp w;
    w.q[0]=z.q[0]*x.q[0];
    w.q[1]=((long double)z.q[0]*x.q[0])*0x1.p-64l;
    w.q[1]+=z.q[1]*x.q[0]+z.q[0]*x.q[1];
    return w;
}

const uint64_t TENPOW19 = 10000000000000000000ULL;

int128tmp a64to128binmult(uint64_t a, uint64_t b){
    int128tmp w;
    w.q[0]=a*b;
    w.q[1]=((long double)a*b)*0x1.p-64l;
    return w;
}

binary128tmp powpow10p[] = {
 {{0x0000000000000000,0xa000000000000000,},    3,},
 {{0x0000000000000000,0xc800000000000000,},    6,},
 {{0x0000000000000000,0x9c40000000000000,},   13,},
 {{0x0000000000000000,0xbebc200000000000,},   26,},
 {{0x0000000000000000,0x8e1bc9bf04000000,},   53,},
 {{0xf020000000000000,0x9dc5ada82b70b59d,},  106,},
 {{0x3cbf6b71c76b25fb,0xc2781f49ffcfa6d5,},  212,},
 {{0xc66f336c36b10137,0x93ba47c980e98cdf,},  425,},
 {{0xddbb901b98feeab8,0xaa7eebfb9df9de8d,},  850,},
 {{0xcc655c54bc5058fa,0xe319a0aea60e91c6,}, 1700,},
 {{0x650d3d28f18b50d1,0xc976758681750c17,}, 3401,},
 {{0xa74d28ce329ace57,0x9e8b3b5dc53d5de4,}, 6803,},
 {{0xc94c153f804a4a9e,0xc46052028a20979a,},13606,},
};

binary128tmp powpow10m[] = {
 {{0xcccccccccccccccd,0xcccccccccccccccc,},-    4,},
 {{0x3d70a3d70a3d70a4,0xa3d70a3d70a3d70a,},-    7,},
 {{0xd3c36113404ea4a9,0xd1b71758e219652b,},-   14,},
 {{0xfdc20d2b36ba7c3e,0xabcc77118461cefc,},-   27,},
 {{0x4c2ebe687989a9b6,0xe69594bec44de15b,},-   54,},
 {{0x67de18eda5814af6,0xcfb11ead453994ba,},-  107,},
 {{0x3f2398d747b3622b,0xa87fea27a539e9a5,},-  213,},
 {{0xac7cb3f6d05ddbf1,0xddd0467c64bce4a0,},-  426,},
 {{0xfa911155fefb5328,0xc0314325637a1939,},-  851,},
 {{0x7132d332e3f20504,0x9049ee32db23d21c,},- 1701,},
 {{0x87a601586bd3f703,0xa2a682a5da57c0bd,},- 3402,},
 {{0x492512d4f2ead3d9,0xceae534f34362de4,},- 6804,},
 {{0x2de38123a1c3d1af,0xa6dd04c8d2ce9fde,},-13607,},
};

binary128tmp operator*(binary128tmp N, binary128tmp M){
    uint64_t w[4] = {0,0,0,0,};
    int128tmp tmp = a64to128binmult(N.m[0],M.m[0]);
    w[0]=tmp.q[0]; w[1]=tmp.q[1];
    uint64_t z;
    tmp = a64to128binmult(N.m[0],M.m[1]);
    z = w[1]+tmp.q[0];
    if(z<w[1]) {w[2]++;}
    w[1]=z;
    z = w[2]+tmp.q[1];
    if(z<w[2]) {w[3]++;}
    w[2]=z;
    tmp = a64to128binmult(N.m[1],M.m[0]);
    z = w[1]+tmp.q[0];
    if(z<w[1]) {w[2]++;}
    w[1]=z;
    z = w[2]+tmp.q[1];
    if(z<w[2]) {w[3]++;}
    w[2]=z;
    tmp = a64to128binmult(N.m[1],M.m[1]);
    z = w[2]+tmp.q[0];
    if(z<w[2]) {w[3]++;}
    w[2]=z;
    z = w[3]+tmp.q[1];
    w[3]=z;
    short exp = N.e+M.e;
    if(w[3]<(1ULL<<63)){
        w[3]<<=1; 
        for(int i=0; i<3; i++){
          w[3-i]|=(w[2-i]>>63); w[2-i]<<=1;
        }
    }
    else exp++;
    if(w[1]>(1ULL<<63)||(w[1]==(1ULL<<63)&&(w[0]>0||(w[2]%2)))){
       w[2]++; if(w[2]==0) {w[2]=0; w[3]++; if(w[3]==0){
         w[3]=1ULL<<63; w[2]=0; exp++;
       }}
    }
    binary128tmp out = {{w[2],w[3],},exp,};
    return out;    
}

long double stringtolongdouble(std::u16string num){
 int i = 0;
 bool sign = 0;
 if(num.size()==0) return 0.l;
 if(num[0]==u'-') {sign=1;i++;} if(num[0]==u'+') i++;
 long double szero = sign ? -0.0l : 0.0l;
 if(num.size()-i>=3&&(num[i+0]&0xFFDF)=='I'&&(num[i+1]&0xFFDF)=='N'&&(num[i+2]&0xFFDF)=='F')
 return 1.0l/szero;
 if(num.size()-i>=3&&(num[i+0]&0xFFDF)=='N'&&(num[i+1]&0xFFDF)=='A'&&(num[i+2]&0xFFDF)=='N')
 return 0.0l/szero;
 int rndmod = fegetround();
 fesetround(FE_TOWARDZERO);
 char digitregister[38];
 for(int i=0; i<38; i++) digitregister[i]=0;
 int r = 0;
 int ex = -1;
 while(i<num.size()&&(num[i]==u'0')) i++;
 while(i<num.size()&&(num[i]>=u'0'&&num[i]<=u'9')&&r<38){
  digitregister[r]=num[i]-u'0'; r++; i++; ex++;
 }
 if(i<num.size()&&num[i]==u'.'){
  i++;
  if(r==0) while(i<num.size()&&(num[i]==u'0')) {i++; ex--;}
  while(i<num.size()&&(num[i]>=u'0'&&num[i]<=u'9')&&r<38){
   digitregister[r]=num[i]-u'0'; r++; i++;
  }
 }
 while(i<num.size()&&(num[i]>=u'0'&&num[i]<=u'9')) i++; //ignore over 38 digits
 if(r==0) {fesetround(rndmod);return szero;}
 if(i<num.size()&&(num[i+0]&0xFFDF)=='E'){
  i++; bool sign2 = 0;
  if(i<num.size()) {if(num[i+0]==u'-') {sign2=1;i++;} if(num[i+0]==u'+') i++;}
  int ex2 = 0;
  while(i<num.size()&&(num[i]>=u'0'&&num[i]<=u'9')) {ex2*=10; ex2+=num[i]-u'0'; i++;}
  if(sign2) ex2=-ex2; ex+=ex2;
 }
 binary128tmp z;
 {
  uint64_t s = 0;
  for(int j=0; j<19; j++) {s*=10; s+=digitregister[j];}
  int128tmp r = a64to128binmult(s,TENPOW19);
  s=0; for(int j=19; j<38; j++) {s*=10; s+=digitregister[j];}
  if(r.q[0]+s<r.q[0])r.q[1]++; r.q[0]+=s;
  z.e=127;
  for(int e=2; e>=0; e--)
  if(!(r.q[1]>>(64-(1<<e)))){r.q[1]<<=(1<<e);r.q[1]|=(r.q[0]>>(64-(1<<e)));r.q[0]<<=(1<<e); z.e-=(1<<e);}
  z.m[0]=r.q[0]; z.m[1]=r.q[1];
 }
 binary128tmp x = {{0,1ULL<<63,},0,};
 ex-=37;
 if(ex>0) for(int i=13; i>=0; i--){if(((+ex)>>i)&1)x=x*powpow10p[i];}
 if(ex<0) for(int i=13; i>=0; i--){if(((-ex)>>i)&1)x=x*powpow10m[i];}
 x=x*z;
 fesetround(rndmod);
 if(x.e<-16446) return szero;
 if(x.e==-16446){if(x.m[0]&&!(x.m[1]&1)) x.m[1]++; x.m[0]=x.m[1]; x.m[1]=0;}
 if(x.e<-16382&&x.e>-16446) {
   uint64_t y = -16382-x.e;
   bool flag = 0;
   if((x.m[0]&((1ULL<<y)-1))&&!((x.m[0]>>y)&1)) flag = 1;
   x.m[0]>>=y; x.m[0]|=x.m[1]<<(64-y); x.m[1]>>=y;
   if(flag) {uint64_t p = x.m[0]+1; if(p<x.m[0])x.m[1]++; x.m[0]=p;}
 }
 if(x.m[0]>(1ULL<<63)||(x.m[0]==(1ULL<<63)&&(x.m[1]&1))) {x.m[1]++; if(x.m[1]==0) {x.m[1]=1ULL<63; x.e++;}}
 if(x.e<-16382) x.e=-16383;
 if(x.e>16383) return 1.0l/szero;
 long double out;
 *(uint64_t*)&out = x.m[1];
 *((short*)&out+4) = (x.e+16383)|((int)sign<<15);
 return out;
}

#include <iostream>
#include <iomanip>
#include <math.h>
int main(){
    std::u16string a = u"4e-4951";
    long double b = stringtolongdouble(a);
    std::cout << b;
}

\$\endgroup\$
5
  • \$\begingroup\$ I am well versed in string to FP, yet the many naked magic numbers, lack of comments and no test harness make for a tedious review during the holidays. UV for a noble attempt (it is a big task no matter what) and UV for @user673679's good review. I look forward to a 2nd more legible version. \$\endgroup\$ Dec 28, 2021 at 0:04
  • \$\begingroup\$ Why <math.h> and not <cmath>? \$\endgroup\$ Dec 28, 2021 at 0:06
  • \$\begingroup\$ Also check out the next follow up, codereview.stackexchange.com/questions/272356/… . It does conversion both ways and supports multiple floating point formats up to quadruple precision. \$\endgroup\$
    – user253344
    Dec 28, 2021 at 6:15
  • \$\begingroup\$ @chux-ReinstateMonica What do you mean by UV? \$\endgroup\$
    – user253344
    Dec 28, 2021 at 8:46
  • \$\begingroup\$ I've rolled back your change to title - we don't use "tags" in question titles like that here. If you feel that your code is completely reviewed, you should Accept an answer. \$\endgroup\$ Jan 7, 2022 at 10:33

1 Answer 1

6
\$\begingroup\$

Formatting:

Formatting is important for readability.

  • Use spaces around operators.
  • Add an empty line between sections of code (like paragraphs).
  • Be consistent with indenting.
  • Indent if branches properly.
  • Don't put too many statements on one line.
  • 1 space is insufficient for indenting.

Tabs are ideal for indenting for accessibility reasons.

Running the code through an auto-formatter to make it legible is an extra barrier to reading and understanding (and reviewing) it.


Fix compiler warnings:

I get the following warnings when compiling the code:

review.cpp(36): error C2220: the following warning is treated as an error
review.cpp(36): warning C4244: '=': conversion from 'long double' to 'uint64_t', possible loss of data
review.cpp(47): warning C4244: '=': conversion from 'long double' to 'uint64_t', possible loss of data
review.cpp(183): warning C4456: declaration of 'i' hides previous local declaration
review.cpp(164): note: see declaration of 'i'
review.cpp(187): warning C4018: '<': signed/unsigned mismatch
review.cpp(189): warning C4018: '<': signed/unsigned mismatch
review.cpp(191): warning C4244: '=': conversion from 'int' to 'char', possible loss of data
review.cpp(196): warning C4018: '<': signed/unsigned mismatch
review.cpp(200): warning C4018: '<': signed/unsigned mismatch
review.cpp(205): warning C4018: '<': signed/unsigned mismatch
review.cpp(207): warning C4244: '=': conversion from 'int' to 'char', possible loss of data
review.cpp(212): warning C4018: '<': signed/unsigned mismatch
review.cpp(219): warning C4018: '<': signed/unsigned mismatch
review.cpp(223): warning C4018: '<': signed/unsigned mismatch
review.cpp(234): warning C4018: '<': signed/unsigned mismatch
review.cpp(252): warning C4456: declaration of 'r' hides previous local declaration
review.cpp(185): note: see declaration of 'r'
review.cpp(266): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
review.cpp(268): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
review.cpp(283): warning C4456: declaration of 'i' hides previous local declaration
review.cpp(164): note: see declaration of 'i'
review.cpp(289): warning C4456: declaration of 'i' hides previous local declaration
review.cpp(164): note: see declaration of 'i'

Some of these are routine "best-practices" stuff (not shadowing variable names), and some of them are potentially more dangerous. Either way, they all need looking at and fixing.

If you don't get warnings like these, turn up your compiler warning level, and turn on "warnings as errors".


Documentation and comments: What does it do? How does it do it?

I think users would have a lot of obvious questions about this code, including:

  • What platforms does it support?
  • What happens when conversion fails or is incomplete?
  • What string format does it support?

Code like this (terse, math heavy) needs comments and documentation. While it's sometimes frowned upon to state simply "what the code does", it's actually essential when the meaning isn't obvious.

To pick a few random lines of code:

long double out;
*(uint64_t *)&out = x.m[1];
*((short *)&out + 4) = (x.e + 16383) | ((int)sign << 15);
return out;

At a glance, I have no idea what this does! and in a year or two I bet you won't either. Technically in C++, you're invoking undefined behavior here (casting and assigning between different pointer types). If there's some implementation specific details that mean it works on certain platforms and compilers, you need to point that out to the reader.

In C++ you can put some of that "documentation" in the code itself as static_asserts. (e.g. checking that the size of long double is 64 bits, or that IEEE 754 floats are supported)).


Unit tests:

Unit tests are essential for something like this, which is so complicated and has so many edge-cases.


Function signature:

long double stringtolongdouble(std::u16string num)

This makes an unnecessary copy of the string. We could pass it by const& to avoid that.

num is not a useful name. input or in_str or would be more descriptive.

std::u16string is a weird choice for the input format. It makes accessing the characters harder. I don't think we need any characters out of the ASCII range here, so we could take a simple std::string_view. Users should convert utf-16 strings to utf-8 or ASCII before calling the function. This is much simpler than hard-coding the conversion from utf-16 inside the parsing function.

The function can fail for many inputs. Returning zero on failure is a terrible idea because it gives us no way to detect the failure. We could throw, return a std::optional<long double>, or return a struct containing the result and any error conditions (e.g. see std::from_chars).


Use C++ standard headers:

#include <stdint.h>
#include <fenv.h>
#include <math.h>

These are C headers that technically may not be present in C++. The C++ headers we need are:

#include <cstdint>
#include <cfenv>
#include <cmath>

Note that these versions define their types (etc.) in the std namespace, not in the global one. So we should use std::uint64_t instead of uint64_t.


Avoid implicit conversion operators:

struct int128tmp{
 uint64_t q[2];

 operator uint64_t(void){
  return q[0];
 }

 operator long double(void){
  return q[1]*0x1.p64l+q[0];
 }
};

Conversion operators should always be explicit in C++. We could avoid these entirely and use named member functions (e.g. to_uint64_t()) to avoid accidents and make it more obvious to the reader.

Since these functions don't change member data, they should be marked const.

Note that declaring functions that take no arguments as (void) is a C thing. It's not necessary or desirable in C++.


stringtolongdouble():

Going through the function line-by-line:


 int i = 0;

This is used to iterate through the string. So it should be a std::u16string::size_type, which is std::size_t. Note that using std::string_view, we wouldn't need this variable. :)


 bool sign = 0;

Use an enum instead for clarity. sign == true being a negative number is somewhat obscure.


 if(num.size()==0) return 0.l;

Again, don't return zero for an error! throw or return an empty optional or something.


 if(num[0]==u'-') {sign=1;i++;} if(num[0]==u'+') i++;

std::string_view gives us starts_with() and remove_prefix() functions that we could use instead. We should also strive to initialize the sign variable at a single point, instead of initializing and then conditionally changing it. (i.e. we should abstract parsing the sign into a function).


 long double szero = sign ? -0.0l : 0.0l;
 if(num.size()-i>=3&&(num[i+0]&0xFFDF)=='I'&&(num[i+1]&0xFFDF)=='N'&&(num[i+2]&0xFFDF)=='F')
 return 1.0l/szero;

Already mentioned: the indentation is incorrect. Manual conversion from utf-16 here should be avoided.

Note that standard C++ parsing with strtold says that the "inf" string is case-insensitive (so "INF", "inf", "Inf" etc. are all recognised). Should we support that? Either way, we need to document the string format!

It's best to avoid division by zero. We can use std::numeric_limits<long double>::infinity() to get the result we need.


 if(num.size()-i>=3&&(num[i+0]&0xFFDF)=='N'&&(num[i+1]&0xFFDF)=='A'&&(num[i+2]&0xFFDF)=='N')
 return 0.0l/szero;

As above. I think we want std::numeric_limits<long_double>::quiet_NaN here to match std::strtold.


 int rndmod = fegetround();
 fesetround(FE_TOWARDZERO);

We don't do any floating point operations until much later... so this should be much later! (Note that we'd avoid having to set it back in an early return if we move it down).

Note that floating point environment access must be turned on with #pragma STDC FENV_ACCESS ON (and / or your platform equivalent), and may not be very well supported.

fesetround can fail, so we need to check the return type.


 char digitregister[38];
 for(int i=0; i<38; i++) digitregister[i]=0;

digitregister is declared earlier than necessary.

Make the magic number 38 a named constant (e.g. max_digits).

A C-style array can be initialized to all zeros like so: char digitregister[38] = {};. But perhaps it would be better to track the number of entries used instead?

In C++ we can use a std::array<char, max_digits> instead of a C-style array.


 int r = 0;
 int ex = -1;
 while(i<num.size()&&(num[i]==u'0')) i++;

r and ex are declared earlier than necessary.

Consider abstracting the matching and parsing of a single character into a function (we're doing the same thing earlier to match the sign). With std::string_view, we could do something like:

bool match(std::string_view& input, std::string_view str)
{
    if (!input.starts_with(str))
        return false;

    input.remove_prefix(str.size());
    return true;
}

So we don't need to worry about checking the size and incrementing i every time. Discarding leading zeros can then be done with:

while (match(input, "0"))
    ;

 while(i<num.size()&&(num[i]>=u'0'&&num[i]<=u'9')&&r<38){
  digitregister[r]=num[i]-u'0'; r++; i++; ex++;
 }

Again, 38 should be a named constant. I don't think we need ex yet (and we don't need to increment it inside the loop) - we can initialize it afterwards.

We could also write a function similar to match() (above) to simplify parsing a value, which might result in clearer code:

auto c = char{};
while (digit_count != max_digits && match_range(input, { '0', '9' }, c))
    digits[digit_count++] = (c - '0');

 if(i<num.size()&&num[i]==u'.'){
  i++;
  if(r==0) while(i<num.size()&&(num[i]==u'0')) {i++; ex--;}
  while(i<num.size()&&(num[i]>=u'0'&&num[i]<=u'9')&&r<38){
   digitregister[r]=num[i]-u'0'; r++; i++;
  }
 }

As above, we could use a match() and match_range() function to simplify this.


 while(i<num.size()&&(num[i]>=u'0'&&num[i]<=u'9')) i++; //ignore over 38 digits

This might be reasonable for digits after the decimal point (though it should be clearly documented) as we're discarding additional precision, but I don't think it makes sense at all if we haven't found a decimal point yet.

If we want to return a partial result, we should at least indicate that we're doing so (see std::from_chars, which returns a pointer to the first unconverted character, so the user can check if the entire input was converted).


 if(r==0) {fesetround(rndmod);return szero;}

As noted, we didn't need to set the rounding mode yet.

Again, returning zero for an error is not a good idea.

By this point, we might need to do some more checking, depending on the string format we support (which is what exactly?). e.g. Are the following strings valid numbers: "", "E", "E5", "+." etc.?


 if(i<num.size()&&(num[i+0]&0xFFDF)=='E'){
  i++; bool sign2 = 0;
  if(i<num.size()) {if(num[i+0]==u'-') {sign2=1;i++;} if(num[i+0]==u'+') i++;}
  int ex2 = 0;
  while(i<num.size()&&(num[i]>=u'0'&&num[i]<=u'9')) {ex2*=10; ex2+=num[i]-u'0'; i++;}
  if(sign2) ex2=-ex2; ex+=ex2;
 }

If we abstracted the sign parsing earlier, we could reuse that function!

I think ex and ex2 can easily overflow here resulting in undefined behavior.


 binary128tmp z;
 {
  uint64_t s = 0;
  for(int j=0; j<19; j++) {s*=10; s+=digitregister[j];}

Why 19? Do you mean std::numeric_limits<std::uint64_t>::digits10?


Unfortunately this is where I run out of time to review any further.

The rest of the code is math heavy, with plenty of magic numbers, and no explanation whatsoever. It needs comments describing what it does and how it does it.

A lot of the code looks like it could be split up into smaller units or named functions that can be tested individually.

P.S.:

if(x.m[0]>(1ULL<<63)||(x.m[0]==(1ULL<<63)&&(x.m[1]&1))) {x.m[1]++; if(x.m[1]==0) {x.m[1]=1ULL<63; x.e++;}}

1ULL < 63 doesn't look right...

\$\endgroup\$
5
  • \$\begingroup\$ "Manual conversion from utf-16 here should be avoided." But how ELSE am I going to parse Unicode strings? \$\endgroup\$
    – user253344
    Dec 27, 2021 at 21:52
  • \$\begingroup\$ if(r==0) {fesetround(rndmod);return szero;} "Again, returning zero for an error is not a good idea." In this specific case, it is actually the case when no non-zero digits have been found, so the number is actually zero. \$\endgroup\$
    – user253344
    Dec 27, 2021 at 21:55
  • \$\begingroup\$ 1) I'd suggest converting from utf-16 as a separate step outside of this function. At the moment this function is effectively converting from utf-16 AND parsing the double from a non-utf-16 format. 2) True. I think I was trying to point out that some strings that should probably be invalid (e.g. "+") will reach here and then return zero too. \$\endgroup\$
    – user673679
    Dec 27, 2021 at 22:03
  • \$\begingroup\$ "Tabs are ideal for indenting for accessibility reasons." --> Spaces make more money. 😉 \$\endgroup\$ Dec 27, 2021 at 23:46
  • \$\begingroup\$ It might seem like "converting" from UTF-16, but it's really parsing it directly. Just like ASCII values can be parsed directly in UTF-8 as bytes, UCS-2 values can be parsed directly in UTF-16 as unsigned short integers. In a software ecosystem where strings are inherently UTF-16, it makes no sense to introduce another encoding just for the string to floating point function. The and with 0xFFDF and comparison with 'I' checks for two values ('i' and 'I'), 0xFFDF is the 16-bit equivalent of the 0xDF that would be used in 8-bit encodings. \$\endgroup\$
    – user253344
    Dec 28, 2021 at 6:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.