Skip to content

Conversation

@dwalluck
Copy link
Contributor

No description provided.

Copy link
Collaborator

@jeremylong jeremylong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jeremylong
Copy link
Collaborator

While I'm okay with this change - I question why there is any implementation of toLowerCase() in the code base. This method might be faster for ASCII strings; however, String.toLowerCase() exists, is highly optimized, handles different locals, etc.

@dwalluck
Copy link
Contributor Author

While I'm okay with this change - I question why there is any implementation of toLowerCase() in the code base. This method might be faster for ASCII strings; however, String.toLowerCase() exists, is highly optimized, handles different locals, etc.

It was to avoid needing to use a Locale as an argument. The previous lack of a Locale in toLowerCase made the lowercase code incorrect on certain other non-English locales. ASCII lowercase should be locale-independent anyway.

The String::toLowerCase for ASCII (actually, Latin1) does some similar bitwise operations, but not the same. Performance should be similar. By the way, Google Guava has something similar https://github.com/google/guava/blob/master/guava/src/com/google/common/base/Ascii.java#L407-L422, so I assume there is a good reason for it.

@ppkarwasz
Copy link
Contributor

I guess this is something the specification should specify. For the general PURL spec, the elements that should be lowercased need also to be ASCII, so there is no problem there.
A problem might appear if some ecosystems allow arbitrary letters in namespaces and names.

@jeremylong
Copy link
Collaborator

Merging this - as the discussion should be a question for the spec. The PR is just a minor change in existing code.

@jeremylong jeremylong merged commit 075cd91 into package-url:master Mar 20, 2025
3 checks passed
@dwalluck dwalluck deleted the fix-to-lowercase branch April 15, 2025 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants