garydgregory commented on issue #26: [BCEL-317] Pluggable cache for ConstantUtf8 URL: https://github.com/apache/commons-bcel/pull/26#issuecomment-496505333 I'm not sure if this PR is a good thing. I'd like feedback from the community. **TL;DR** If you want caching, use the existing `getCachedInstance()` API, it's an existing feature. **Details** [Please do not hesitate to correct details I may have wrong] Right now, before this PR, we have two clear and simple APIs: - `getInstance()` always returns a new instance. - `getCachedInstance()` returns a cached instance unless the cache is full. Some thoughts: - The PR adds 2 global variables as system properties, this is definitely not a good thing, even if the current code already uses a sys prop. - The PR removes an existing sys prop, therefore breaking compatibility. - The PR changes the behavior of `getInstance()` which can now return a cached instance, which could (or not) break things but definitely falls into the unexpected category IMO. **More Details** The PR should not write to the console. The existing code might be better re-implemented using a ConcurrentHashMap for simpler synchronization.
---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
