Community
Participate
Working Groups
Build Identifier: 20090920-1017 class Bug {{ final Runtime runtime = Runtime.getRuntime(); runtime.getClass() // LINE 3 .toString(); runtime.getClass().hashCode(); }} Reproducible: Always Steps to Reproduce: Please try to extract a temporary from the expression on LINE 3. See that the duplicate expression in the next statement is wrongly missed.
Minimal example: class Try { { getClass().equals(getClass()); } } The two calls to Class#getClass() resolve to different bindings, which makes JdtASTMatcher think the fragments don't match. Moving to Core, since the two bindings are not identical, but their keys are the same and isEqualTo(..) returns true. I expect identical bindings.
Note that this is only a problem for the getClass() method with it's special return type (JLS 4.3.2). Works fine for other cases e.g. this: public class Try { { get().equals(get()); } protected Class<? extends Try> get() { return getClass(); } }
Created attachment 158163 [details] Patch under consideration
Looks good except that I would put the regression test inside the org.eclipse.jdt.core.tests.dom.ASTConverter15Test tests.
Created attachment 158258 [details] Proposed fix + regression test Updated patch with test moved to ASTConverter15Test
Created attachment 158259 [details] Proposed fix + regression test Same patch with updated copyright
Thanks for the review & reorganization. Released in HEAD for 3.6M6.
Sorry, I'm a little late, but are these WeakReferences really necessary? The straightforward way to solve this would be to just add a field to TypeBinding where you can cache the special method binding. If you don't want that for memory space reasons, I think the cache should at least be created in the lookup environment so that the space in the WeakHashMap can be reclaimed when the environment is GC'd. I think the current code also has a concurrency issues, since the unsynchronized map can be accessed from multiple threads. I think you could even get away without a cache. After all, isn't this just another method that could be fetched from the lookup environment (and gets cached there via exisitng lookup mechanisms)?
(In reply to comment #8) > Sorry, I'm a little late, but are these WeakReferences really necessary? The > straightforward way to solve this would be to just add a field to TypeBinding > where you can cache the special method binding. This approach was considered but dismissed (for the same reasons you identify) > I think the current code also > has a concurrency issues, since the unsynchronized map can be accessed from > multiple threads. Good point. I was less worried about the reclamation of the WeakHashMap itself (as long as the entries are reclaimed suitably) but while I rework this fix to address the concurrency issue, I'll also see if I can logically group this cache with the existing others in LookupEnvironment and eliminate any hangovers.
Created attachment 158418 [details] Revised patch Revised patch under test.
Even better. +1.
Reworked patch released to HEAD for 3.6M6.
verified for 3.6M6 using build I20100305-1011.
Verified.
