compare_numeric() now ignores leading zeros of any numbers embedded in StringRef. Previously, a longer number would always be considered larger than the shorter number. For example, "007" would be considered larger than "8". The behaviour is now more intuitive.
This adds lots of code and makes the code harder to read in order to handle this case.
Is there a specific problem you're addressing here? Can you work to make the code more readable rather than less readable if we need to support this use case?
Also, this ignores the common usage of a leading zero to indicate octal. I'm somewhat worried about StringRef having divergent behavior there.
Thanks for the review.
I'm not addressing a specific problem here; merely going off on the
comment mentioning that prefixed zeroes didn't work very well. I
gather that you'd argue against merging in this case, even if the code
were more readable. (I'd be happy to work on the readability, though.)
Regarding octal numbers: I believe behaviour is unchanged. Comparison
between two octal numbers (with or without leading zeroes) should work
as expected. Exactly which behaviour are you worried about?
Improved readability by using strtol(), which handles the conversion to numericals. It automatically handles prefixes of 0x for hexadecimal and 0 for octal.
Should be more readable than the original, as well. :)
This is a complex change to a routine with a strange and unclear interface and purpose.
In fact, this code is only called in one place that I can find outside of its unit tests: include/llvm/TableGen/Record.h. I'm not sure what that code is trying to do, but I don't think adding more complexity here to handle a corner case is the right path forward. I suspect that code should be rolling its own (hopefully *simpler* and more domain specific) logic and this method should just be deleted. But that doesn't really seem critically important either.
What I am confident in is that we shouldn't burn complexity and risk of bugs (or performance hits) on trying to make this function handle leading zeros particularly gracefully.
I believe it's current usage in TableGen I believe is to make record names come out in order like this.
I'm not sure it matters to support octal or hex in a context where it's embedded in string with letters in it. It would definitely be weird for hex.
For the tablegen use case I think if someone were to put in extra zeros in their tablegen recrds they would only do so to put a consistent number of digits in all records with a common prefix string. So I don't think supporting leading zeros gracefully is necessary.