nfsantos commented on PR #2178: URL: https://github.com/apache/jackrabbit-oak/pull/2178#issuecomment-2721185741
> The tricky questions remain: should we review the code to check what semantics are really needed (a bit a dangerous, does not scale well, but may clarify the code). should we check whether performance really matters? As these changes are intended to be purely mechanical, I don't think we should be looking at how the objects are used. That does not scale and it is very error-prone, as we may miss some hidden assumptions about the behaviour of the APIs that are being changed. The safest way would be to preserve the semantics and performance characteristics as much as possible. So in this case, the Set should still be immutable and should have similar performance and memory usage. Replacing the Guava sets by the sets created by `Set.of()` would be closer to the original semantic and performance characteristics. > FWIW, when I replaced other collection utilities, I did exacly that, and apparently did not break anything. See, for instance, https://issues.apache.org/jira/browse/OAK-11278. Note that in this case, I did the zero/one/more variants separately (evidently zero or one arguments are easier to argue about). This PR replaces `ImmutableMap` with `Map.of()`, which are much closer, both are immutable and have similar performance characteristics. But it was still dangerous because the iteration behaviour of the maps created by `Map.of()` is not the same as the ones created by `ImmutableMap`, and some code may rely on a particular iteration order, even though this is not guaranteed by the Map spec. This is a subtle change which may not be caught in testing, and may end up causing hard to debug problems in production. -- 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]
