Repository navigation
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the
if (cp < 0xb5)case below seems good to me, but removing the fast-path for the already-folded region [0x61,0x7a] seems wrong. Is there a reason for it, or was it a mistake?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I thought about it and it was a deliberate choice to remove it. But it's not important. Important is that there is no more std::lower_bound call for each space, brace, etc, as it was in the past. I am fine both with the fast path and without it.
If you are interested, here is my reasoning. Only read if you really want to know:
I considered six cases and counted the number of comparisons for each case.
Three different classes of characters:
And two code options:
A: with fast-path for [0x61,0x7a]
B: without fast-path for [0x61,0x7a]
Now let's count the comparisons:
First uppercase A-Z:
Case 1-A:
0x61(false),0x41(true),0x5a(true)
Case 1-B:
0x41(true),0x5a(true)
-> The fast path for lowercase a-z costs us one comparison for each uppercase A-Z
Then lowercase a-z:
Case 2-A:
0x61(true),0x7a(true)
Case 2-B:
0x41(true),0x5a(false),0xb5(true)
-> The fast path for lowercase a-z saves us one comparison for each lowercase a-z
Then punctuation/whitespace:
Case 3-A:
0x61(false),0x41(false),0xb5(true)
Case 3-B:
0x41(false),0xb5(true)
-> The fast path for lowercase a-z costs us one comparison for each punctuation or whitespace
Conclusion:
The fast path for a-z saves us one comparison for each lowercase a-z character, but costs us one comparison for every uppercase character, every punctuation, brace, space etc. Looking at my vba code, I had the impression that there are not more a-z characters than other characters. Combined with my fundamental believe "less code is better" I decided to remove the fast-path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reading my own comment I realized we can do even better:
#335