Page MenuHomePhabricator

Fix 'leading zeros' caveat of StringRef::compare_numeric()
Needs RevisionPublic

Authored by snnw on Feb 3 2015, 1:11 PM.

Details

Reviewers
chandlerc
Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

snnw updated this revision to Diff 19260.Feb 3 2015, 1:11 PM
snnw retitled this revision from to Fix 'leading zeros' caveat of StringRef::compare_numeric().
snnw updated this object.
snnw edited the test plan for this revision. (Show Details)
snnw added a reviewer: chandlerc.
snnw set the repository for this revision to rL LLVM.
snnw added a subscriber: Unknown Object (MLST).
chandlerc requested changes to this revision.Mar 29 2015, 4:47 PM
chandlerc edited edge metadata.

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.

This revision now requires changes to proceed.Mar 29 2015, 4:47 PM
snnw added a comment.Mar 30 2015, 6:26 AM

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?

snnw updated this revision to Diff 23274.Apr 6 2015, 8:16 AM
snnw edited edge metadata.

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

chandlerc requested changes to this revision.May 27 2015, 1:31 AM
chandlerc edited edge metadata.

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.

This revision now requires changes to proceed.May 27 2015, 1:31 AM

I believe it's current usage in TableGen I believe is to make record names come out in order like this.

XMM0
XMM1
XMM2
...
XMM9
XMM10

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.