vy commented on code in PR #2256:
URL: https://github.com/apache/logging-log4j2/pull/2256#discussion_r1470759739
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/impl/JdkMapAdapterStringMapTest.java:
##########
@@ -44,6 +44,7 @@ public class JdkMapAdapterStringMapTest {
@Test
public void testConstructorDisallowsNull() {
assertThrows(NullPointerException.class, () -> new
JdkMapAdapterStringMap(null));
Review Comment:
[Optional] I would remove this line using the deprecated ctor.
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/impl/JdkMapAdapterStringMapTest.java:
##########
Review Comment:
[Optional] I would also replace the use of the deprecated ctor in the
`testImmutability()` method too.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/impl/JdkMapAdapterStringMap.java:
##########
@@ -47,9 +47,14 @@ public class JdkMapAdapterStringMap implements StringMap {
private transient String[] sortedKeys;
public JdkMapAdapterStringMap() {
- this(new HashMap<String, String>());
+ this(new HashMap<String, String>(), false);
}
+ /**
+ * @deprecated for performance reasons since 2.23.
+ * Use {@link #JdkMapAdapterStringMap(Map, boolean)} instead.
+ */
+ @Deprecated
public JdkMapAdapterStringMap(final Map<String, String> map) {
this.map = Objects.requireNonNull(map, "map");
try {
Review Comment:
I agree that the new ctor explicitly requiring an immutability flag is the
way to go forward, though I feel uncomfortable leaving this deprecated ctor as
is. This try-catch is the problem, yet we work around it rather than fixing it.
Can't we do something else here? For instance,
```
if (knownImmutableMapClasses.contains(map.getClass())) {
immutable = true;
} else {
// try-catch
}
```
--
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]
