Remove undefined behavior and add stricter checks in console arg parsing

[kunaltyagi]

Started with a wish to reduce duplication, but grew into a monster than has better semantics, stricter bound checking and fewer UB along with less duplicated code.

The original implementation had several silent failures (on edge cases) such as

• use of atoi and atol
  • returns 0 on failure to parse (wrong input)
  • no bounds checked for unsigned int (negative numbers became large positive numbers)
• non-handling of edge case where sizeof (int) == sizeof (long int) for unsigned int

Step 1: Use strtoX

Usage of strtoX allowed error handling, but needed to be separated in a template-function to prevent duplication of logic. 3 sub-cases were needed for minimal overhead:

• double, float, long int were simple enough
• integral types (int, unsigned int) needed bounds checked
• boolean was a simple indirection

Step 2: Check bounds for integral types

It was still possible to have just 2 simple functions, but

• It would have required duplicating the checks (which I already messed up once) apart from being non-intuitive. So I introduced a template-function hop

Step 3: Now do it in cross-platform manner

This worked for LP64, but failed on LLP64. Current solution should work with all 64-bit memory models.

• The edge case on LLP64 required the jumping through TMP hoops to get the right type. This is a clean-ish solution (another method was using std::conditional all over, which I didn't like).
• Using MPL was a bit overkill for this situation, even though the solution would have been a tad bit cleaner
• Using if constexpr wasn't an option in C++14

Final implementation

100 lines which implement:

• a generic function to encapsulate C style errorno checking
• a generic function to encapsulate integral bounds checking
• TMP to find the correct type for various 64 bit system implementations

And @larshg was nice enough to debug the platform dependent issue. Wouldn't have caught it for a few more days without him

[kunaltyagi]

I'll remove the use of atoi and atof functions with strtol and strtod (which aren't UB footguns) to have better semantics.

[SergioRAgostinho]

Besides my really minor comment, LGTM. Up to you do decide to address it or not. Feel free to merge after.

[SergioRAgostinho]

I just realized the CI is red :')

[larshg]

Hi @kunaltyagi

The cast of max to long int gives -1:

I tried to change it to long unsigned int - but that didn't work well with the -314 case :)

[kunaltyagi]

The cast of max to long int gives -1:

That's a nice find. I should have seen it coming. Dang it Windows. At least it tells me I need to change my SFINAE. Thanks for the hint @larshg

[larshg]

What does it return linux/unix side ? Max of singed int? ie. 2,147,483,647 or?

[kunaltyagi]

On my system, sizeof(int) < sizeof(long int).

[larshg]

Ahh, yeah its equal here.
According to this, the use of long long int should do it. And the test passes here.

[kunaltyagi]

Oh well, now I can spend atleast 1 PR strengthening the parse.cpp itself

[kunaltyagi]

@larshg Since you have a Windows PC, could you check the latest push? It compiles for me without issues.

[larshg]

👍 yes, it compiles and unit tests passes.

[SergioRAgostinho]

At first sight this feels slightly over engineered :') Thank you for chiming in @larshg

[kunaltyagi]

this feels slightly over engineered

Yeah, that's true. Net result: ~300 lines added. Positive side: 200 lines are tests.

The original implementation had several silent failures (on edge cases) such as

• use of atoi and atol
  • returns 0 on failure to parse (wrong input)
  • no bounds checked for unsigned int (negative numbers became large positive numbers)
• non-handling of edge case where sizeof (int) == sizeof (long int) for unsigned int

Step 1: Use strtoX

Usage of strtoX allowed error handling, but needed to be separated in a template-function to prevent duplication of logic. 3 sub-cases were needed for minimal overhead:

• double, float, long int were simple enough
• integral types (int, unsigned int) needed bounds checked
• boolean was a simple indirection

Step 2: Check bounds for integral types

It was still possible to have just 2 simple functions, but

• It would have required duplicating the checks (which I already messed up once) apart from being non-intuitive. So I introduced a template-function hop

Step 3: Now do it in cross-platform manner

This worked for LP64, but failed on LLP64. Current solution should work with all 64-bit memory models.

• The edge case on LLP64 required the jumping through TMP hoops to get the right type. This is a clean-ish solution (another method was using std::conditional all over, which I didn't like).
• Using MPL was a bit overkill for this situation, even though the solution would have been a tad bit cleaner
• Using if constexpr wasn't an option in C++14

Final implementation

100 lines which implement:

• a generic function to encapsulate C style errorno checking
• a generic function to encapsulate integral bounds checking
• TMP to find the correct type for various 64 bit system implementations

And @larshg was nice enough to debug the platform dependent issue. Wouldn't have caught it for a few more days without him

[SergioRAgostinho]

I just copied your explanation to the OP. Future people will appreciate it. :)