You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2022/02/03 20:08:47 UTC
[GitHub] [commons-collections] Simulant87 opened a new pull request #276: COLLECTIONS-803: prevent duplicate call to convertKey on put for CaseInsensitiveMap
Simulant87 opened a new pull request #276:
URL: https://github.com/apache/commons-collections/pull/276
Improves the performance by re-using the once converted key again when creating a new entry.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] Simulant87 removed a comment on pull request #276: COLLECTIONS-803: prevent duplicate call to convertKey on put for CaseInsensitiveMap
Posted by GitBox <gi...@apache.org>.
Simulant87 removed a comment on pull request #276:
URL: https://github.com/apache/commons-collections/pull/276#issuecomment-1029455359
@garydgregory Thank you for the feedback.
I added a test the verifies the `convertKey` method is only called once, which fails currently on the master branch without the production code changes provided.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] Simulant87 commented on pull request #276: COLLECTIONS-803: prevent duplicate call to convertKey on put for CaseInsensitiveMap
Posted by GitBox <gi...@apache.org>.
Simulant87 commented on pull request #276:
URL: https://github.com/apache/commons-collections/pull/276#issuecomment-1038825792
@garydgregory Thank you for the feedback.
I added a test that verifies the `convertKey` method is only called once, which fails currently on the master branch without the production code changes provided.
I kindly request another review. Please let me know if there is anything else missing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] Simulant87 commented on pull request #276: COLLECTIONS-803: prevent duplicate call to convertKey on put for CaseInsensitiveMap
Posted by GitBox <gi...@apache.org>.
Simulant87 commented on pull request #276:
URL: https://github.com/apache/commons-collections/pull/276#issuecomment-1029455359
@garydgregory Thank you for the feedback.
I added a test the verifies the `convertKey` method is only called once, which fails currently on the master branch without the production code changes provided.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] Simulant87 commented on pull request #276: COLLECTIONS-803: prevent duplicate call to convertKey on put for CaseInsensitiveMap
Posted by GitBox <gi...@apache.org>.
Simulant87 commented on pull request #276:
URL: https://github.com/apache/commons-collections/pull/276#issuecomment-1050209446
Hello @kinow,
I saw that you are able to merge in this repository in another recent PR. Would you also like to review my suggested changes in this PR, please?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] kinow commented on pull request #276: COLLECTIONS-803: prevent duplicate call to convertKey on put for CaseInsensitiveMap
Posted by GitBox <gi...@apache.org>.
kinow commented on pull request #276:
URL: https://github.com/apache/commons-collections/pull/276#issuecomment-1050253135
> Hello @kinow, I saw that you are able to merge in this repository in another recent PR. Would you also like to review my suggested changes in this PR, please?
Sorry, busy with other pull requests & issues :disappointed_relieved: :bow: code looks good from a brief look, but would need more time to really understand the change and confirm it's a good improvement :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] freya022 commented on pull request #276: COLLECTIONS-803: prevent duplicate call to convertKey on put for CaseInsensitiveMap
Posted by GitBox <gi...@apache.org>.
freya022 commented on pull request #276:
URL: https://github.com/apache/commons-collections/pull/276#issuecomment-1050121979
I may also suggest changing the `convertKey` method to use `key.toString().toLowerCase(Locale.ROOT)` to convert the non-null key, instead of the char loop.
Benefits include better performance (measured a 2 times increase on my pc), and this is more fitted for the use case, as per the [Character#toLowerCase documentation](https://docs.oracle.com/javase/7/docs/api/java/lang/Character.html#toLowerCase(char)):
```
In general, String.toLowerCase() should be used to map characters to lowercase.
String case mapping methods have several benefits over Character case mapping methods.
String case mapping methods can perform locale-sensitive mappings, context-sensitive mappings, and 1:M character mappings, whereas the Character case mapping methods cannot
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] Simulant87 commented on pull request #276: COLLECTIONS-803: prevent duplicate call to convertKey on put for CaseInsensitiveMap
Posted by GitBox <gi...@apache.org>.
Simulant87 commented on pull request #276:
URL: https://github.com/apache/commons-collections/pull/276#issuecomment-1050206636
@freya022 thank you for your suggestion. This sounds like another high performance improvement.
However it also sounds like the behaviour might slightly change in some edge cases. Something I expect the maintainers want to avoid and could cause a longer discussion. Therefore I would like to keep your suggestion separated from my changes. As my implementation does not cahnge the behaviour of the key conversion, it should be simpler to review and approve. I encourage your to create a ticket in [Jira](https://issues.apache.org/jira/projects/COLLECTIONS/issues) and create a separate pull request for that change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] garydgregory commented on pull request #276: COLLECTIONS-803: prevent duplicate call to convertKey on put for CaseInsensitiveMap
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #276:
URL: https://github.com/apache/commons-collections/pull/276#issuecomment-1029397096
Hello @Simulant87
You'll need to add a test case to this PR that shows at least one failing test when run without the main changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org