Add user chosable base support to StringToInt()

[Keeper-of-the-Keys]

Needed for FTDI plugin to create a unique ID for RDM packets based on base36 serial stored in the EEPROM.

[peternewman]

I'm wondering if HexStringToInt should be switched to use this too now?

A few minor improvement requests too please.

[Keeper-of-the-Keys]

@peternewman Unless there is something more you want me to add can you re-trigger travis?

[peternewman]

@peternewman Unless there is something more you want me to add can you re-trigger travis?

As per the other I'd like to get #1589 in so at least some compilation is tested.

[peternewman]

Sorry, I thought I'd submitted this review, but it seems not.

[peternewman]

Please can we switch the strtoul/strtoll bits of HexStringToInt to use this new function?

[Keeper-of-the-Keys]

Please can we switch the strtoul/strtoll bits of HexStringToInt to use this new function?

Leave HexStringToInt() intact or replace the calls to it with StringToInt()?
Also should it be strict because it does not contain such an argument?

Edit: according to a quick grep HexStringToInt() is not used very much (5 usages outside of unit tests and function definitions) so I think just replacing these 5 instances with StringToInt() is the way to go.

Still leaves me with the question are they "strict" or not?

[Keeper-of-the-Keys]

@peternewman I took the liberty to replace HexStringToInt hope you don't mind.

Also looking at its' usage it seemed to me that strict mode was most appropriate (though I guess the way it used to work was non-strict, but the data delivered to it should not be longer then the wanted number string)

[peternewman]

Please can we switch the strtoul/strtoll bits of HexStringToInt to use this new function?

Leave HexStringToInt() intact or replace the calls to it with StringToInt()?
Also should it be strict because it does not contain such an argument?

Edit: according to a quick grep HexStringToInt() is not used very much (5 usages outside of unit tests and function definitions) so I think just replacing these 5 instances with StringToInt() is the way to go.

Still leaves me with the question are they "strict" or not?

I meant just replace the bit of the call within HexStringToInt() that currently calls strtoul/strtoll with your new options to StringToInt(). You've dropped the protection that the check for ABCDEFabcdef0123456789 offered, or is that still covered by the base argument to strtoul/strtoll etc?

Also we can't just remove the function entirely, we need to add a wrapper version of it and mark it as deprecated with a date in the Doxygen in case other people are using it via our libraries.

Personally I'd be tempted to say that converting a hex string to an int is important enough we could keep the wrapper function which makes it a lot more obvious what it's doing without needing to know what the arguments are. I'm rather surprised we only use it five times, although I think that's because the actual calls are wrapped within a UID/MAC FromString function.

[Keeper-of-the-Keys]

I meant just replace the bit of the call within HexStringToInt() that currently calls strtoul/strtoll with your new options to StringToInt().

I actually tried that first but the compiler had a really hard time (ie. failed) to select the right versions of the function and thus failed to compile.

You've dropped the protection that the check for ABCDEFabcdef0123456789 offered, or is that still covered by the base argument to strtoul/strtoll etc?

I'm not sure what you are asking here? base = 16 covers hex in all its' permutations as far as I understand, I copied all the HexStringToInt() unit tests and replaced them to be StringToInt() with base = 16 and as far as I can tell it passes them.

Also we can't just remove the function entirely, we need to add a wrapper version of it and mark it as deprecated with a date in the Doxygen in case other people are using it via our libraries.

Understood but I have no solution for the problem I mentioned above...

Personally I'd be tempted to say that converting a hex string to an int is important enough we could keep the wrapper function which makes it a lot more obvious what it's doing without needing to know what the arguments are. I'm rather surprised we only use it five times, although I think that's because the actual calls are wrapped within a UID/MAC FromString function.

So maybe have a single instance of HexStringToInt() wrapping StringToInt() instead of the 6 different versions we used to have?

[Keeper-of-the-Keys]

@peternewman I see what you mean by asking about the "ABCDEFabcdef0123456789" protection, this would be strict = true in StringToInt() which causes a false to be returned if strtoul/strtoll finished to consume the string anywhere before the end of the string (due to a character not being part of the encoding being consumed).

[peternewman]

I meant just replace the bit of the call within HexStringToInt() that currently calls strtoul/strtoll with your new options to StringToInt().

I actually tried that first but the compiler had a really hard time (ie. failed) to select the right versions of the function and thus failed to compile.

Was that with one function or six?

The different versions are to deal with two things, firstly the max value part, but also how it needs to cast things back and forth, see also various other functions in this file.

[peternewman]

You've dropped the protection that the check for ABCDEFabcdef0123456789 offered, or is that still covered by the base argument to strtoul/strtoll etc?

I'm not sure what you are asking here? base = 16 covers hex in all its' permutations as far as I understand, I copied all the HexStringToInt() unit tests and replaced them to be StringToInt() with base = 16 and as far as I can tell it passes them.

How are you running the tests your end? Via make check? If you look on Travis it's failing them on both compilers.

[Keeper-of-the-Keys]

How are you running the tests your end? Via make check? If you look on Travis it's failing them on both compilers.

Yup as far as I understood the output there are failed checks but they are from a different section, but now that I look at the logs I see that this is not the case 🤦‍♂️

The following tests fail:
OLA_ASSERT_FALSE(StringToInt("0xf", &value, true, 16));
and
OLA_ASSERT_TRUE(PrefixedHexStringToInt("0xffffffff", &value));

It is unclear to me why the first test should fail... 0xf is valid hex so why should the function fail?

The second one I need to be more awake to be able to analyze.

[Keeper-of-the-Keys]

Was that with one function or six?

Six.

The different versions are to deal with two things, firstly the max value part, but also how it needs to cast things back and forth, see also various other functions in this file.

Yeah but for some reason even though the same 6 overloaded versions of StringToInt() exist using them in the HexStringToInt() function caused errors that the compiler could not select the right version needed 🤷‍♂️

[Keeper-of-the-Keys]

OK after looking at this a bit more here are my observations on the 2 test results:

1. This used to be an input resulting in false for the HexStringToInt() function since it only allows 0..F in either case so 0x would fail, however strtol() actually accepts 0x, 0X and 0 as prefixes for base 16 and base 8, thus I think we can remove this test.

2. Since strtol() and thus StringToInt() actually accepts the prefix this whole function can be deprecated, the problem itself seems to be that the wrong overloaded function is selected since earlier tests of the "ffffffff" string have returned -1 (unless the checks stop instantly on the first fail).

[Keeper-of-the-Keys]

PrefixedHexStringToInt() is only used twice:

• examples/ola-rdm.cpp: (ola::PrefixedHexStringToInt(pid_name, &pid_value) ||
• common/rdm/StringMessageBuilder.cpp: ola::PrefixedHexStringToInt(input, &int_value) ||

Also excellent candidate for deprecation & replacement with StringToInt()

[Keeper-of-the-Keys]

🤦‍♂️ When a test fails further tests aren't being run... I'll be pushing an update once I figure out what is failing here...

[Keeper-of-the-Keys]

@peternewman I may be wrong here but I'm pretty sure that the current implementation of StringToInt() has a few mistakes in implementation the most glaring seeming to be that in the "int" output version it is doing a static_cast to unsigned int on the output.

ola/common/utils/StringUtils.cpp

Line 221 in 90c2691

*output = static_cast<unsigned int>(l);

Or is this on purpose?

[Keeper-of-the-Keys]

After reviewing the original code and my code and also looking at the way things function it seems to me that the error in the -1 tests comes from the fact that strtol and strtoll are storing in and dealing with variables that are much larger then any of the ints we are dealing with so when it gets for instance 0xff that is treated as 0x00000000000000ff and not as 0xffffffffffffffff now if the function is checking for > INT8_MAX it will always be true for negative numbers since their sign is currently not a sign bit but the MSB.

The solution seems to be that we should check for > UINT8_MAX, UINT16_MAX etc.

It seems that strtol will basically read the whole sequence of characters as numbers and only a -/+ as the sign for the number from my tests and also from the documentation.

[Keeper-of-the-Keys]

The reason why in the "old" code HexStringToInt() worked just fine and dandy while using StringToInt() to take over its' job fails is that HexStringToInt() never bothers to check whether the resulting value is within bounds or not, it just returns the result of strtoll() or strtoul() respectively and lets the runtime handle type conversion etc.

StringToInt() OTOH is checking whether the resulting value is within bounds or not, then since strtoll() treats the string as a whole as a number and only the minus (-) sign is a sign a negative number provided in an encoding that has a sign bit will always exceed INT*_MAX

@peternewman @nomis52 I don't know how to resolve this we seem to have 3x3x2 different wrappers of strtol/strtoul that implement different logic surrounding the function (and some wrappers of the wrappers).

[Keeper-of-the-Keys]

@peternewman @nomis52 I think I resolved most/all of the logic there is one case of behavior that changes as reflected in 7aef267 however I believe the old behavior was wrong if we wanted 0xffff to be treated as -1 for a int16_t then 0xffffffff should also be treated as such, StringToInt() is not here to determine whether the original string value is of a similar length in it's binary representation but does answer the question whether the number represented by the string would fit in the variable we chose.

[Keeper-of-the-Keys]

Also travis is blowing up on some xenial apt packages...