Conversion from string to long double

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...