Community
Participate
Working Groups
HEAD ASTRewrite should offer get/setProperty() methods like ASTNode. E.g. for bug 319069, I need to modify our VariableDeclarationRewrite#rewriteModifiers(..) to store additional information about nodes that have already been touched by a rewrite. Storing this information externally and rippling it through all APIs where the ASTRewrite is currently passed through is a pain.
I'll follow up.
I'd like to see this fixed for M3 so that we can fix the bug in JDT UI. Thanks.
Created attachment 179620 [details] proposed fix v1.0 Here's the fix to introduce the same get/setProperty functionality as in ASTNode. Olivier, can you please do a quick review? Thanks!
Looks good. I'll attach a new patch with a few cosmetic changes (typo, remove reference to HashMap to keep Map only, add throws clauses in javadoc) + some regression tests. It would be good if Markus could also use the patch to see if this is helping him to solve bug 319069. Looking at the existing patch for 319069, I believe this is good enough.
Created attachment 179676 [details] Proposed fix + regression tests Ayushman, please double check the new patch. If ok, you can release it.
Markus is away until next Monday.
(In reply to comment #5) > Created an attachment (id=179676) [details] [diff] > Proposed fix + regression tests > > Ayushman, please double check the new patch. If ok, you can release it. Looks good to me. I'll release after Markus reviews.
Created attachment 179735 [details] Proposed fix + regression test Renamed the test in the test suite to something more meaningful.
The API looks good. Things that should be changed in the implementation: - Javadoc of ASTRewrite#property1 talks about an unmodifiable map in #property2, but that's never the case (in ASTNode, the unmodifiable map is created lazily in #properties()). - I've compared the implementation with ASTNode and found some changes. These should also be backported into ASTNode so that the implementations match up again. Hint: Search for declarations of methods "org.eclipse.jdt.core.dom*.AST*.?etProperty" and then select the two methods and perform Compare With > Each Other.
(In reply to comment #9) > The API looks good. Things that should be changed in the implementation: > - Javadoc of ASTRewrite#property1 talks about an unmodifiable map in > #property2, but that's never the case (in ASTNode, the unmodifiable map is > created lazily in #properties()). This is not exposed. We would need to add a #properties() method on ASTRewrite if this is needed. Do you want such a method ?
No, I don't need #properties() and I wouldn't add it (ASTNode#properties() is not used anywhere in the SDK). I just saw that the internal Javadoc on the field is wrong.
Created attachment 180334 [details] Proposed fix + regression test This patch should take care of all your comments.
I don't think there is a need to say that property2 is null if property1 is a Map.
I am committing the last patch to get going with this bug. Released for 3.7M3. Added regression test in: org.eclipse.jdt.core.tests.rewrite.describing.ASTRewritePropertyTest. Markus, if you have further comment, we will adjust the code.
Verified for 3.7M3 using I20101025-1602 (4.1 build)
