ppkarwasz commented on code in PR #3868:
URL: https://github.com/apache/logging-log4j2/pull/3868#discussion_r2268805528
##########
log4j-api/src/main/java/org/apache/logging/log4j/CloseableThreadContext.java:
##########
@@ -206,7 +207,7 @@ public void close() {
}
private void closeMap() {
- final Map<String, String> valuesToReplace = new
HashMap<>(originalValues.size());
+ final Map<String, String> valuesToReplace =
Maps.newHashMap(originalValues.size());
Review Comment:
I’d prefer to replace it everywhere. My reasoning is:
These changes simply swap a constructor call for a static helper call.
If `log4j-api` or (more likely) `log4j-core` eventually moves to JDK 21, and
we’ve only inlined the fix in a single spot, it’s very likely we’ll forget
about it. On the other hand, if we use a `Maps` helper class, placed in an
**internal** package per artifact, and document that it should be replaced with
`HashMap.newHashMap` once our baseline is JDK 19+, there’s a much better chance
we’ll catch it.
As I mentioned in
https://github.com/apache/logging-log4j2/pull/3868#issuecomment-3158723587, the
only change I’d like to see in this PR is to hide the helper in an `internal`
package that is **not** exported via OSGi or JPMS, so we can drop it cleanly
when our baseline surpasses JDK 19. This obviously also means that `log4j-api`
and `log4j-core` need a separate copy of the helper.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
