This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
I am rewriting an IfTree using these statements: IfTree original = (IfTree) node; modified = make.If( make.Parenthesized( make.Unary(Kind.LOGICAL_COMPLEMENT, original.getCondition())), original.getElseStatement(), null); System.out.println("original: " + node); System.out.println("modified: " + modified); wc.rewrite(node, modified); For input: void m1(boolean b) { if (b) ; else System.out.println("hi"); if (b) ; else System.out.println("hi"); } I get the output: void m1(boolean b) { if (!(b)) System.out.println("hi"); if (!(b)) { System.out.println("hi"); } } That is, braces are added in the second case but they should not be. Even if you decided to add braces (which is not a good idea in my opinion), it should be consistent. That is, either add or not to add in all cases. The log: original: if (b) ; else System.out.println("hi"); modified: if (!(b)) System.out.println("hi"); original: if (b) ; else System.out.println("hi"); modified: if (!(b)) System.out.println("hi");
.
I'd like to NetFIX [1] this bug. Is it possible? [1] http://wiki.netbeans.org/NetFIX
Created attachment 82246 [details] test case
I think this is INVALID. @see attached test case, which passes for me.
I consider this issue valid. Your patch for test case does not reflect this issue. I'm adding patch to test which covers this issue.
Created attachment 82629 [details] Correct test patch
aah! I see the mistake now.
Ok, looks like CasualDiff is looking at these two actions as DELETE, DELETE, INSERT, INSERT rather than MODIFY, MODIFY. The problem is therefore in the code that decides what to do, rather than the code that does it... if that makes sense.
Created attachment 82693 [details] test case and proposed fix
@moonko, I think this bug has actually shed light on something quite fundamentally broken in the diff code. The Measure.MEMBER was requiring that the Kind and pos of a JTree both be the same before they were considered ALMOST_THE_SAME, whereas THE_SAME_KIND was being ignored. I've updated the code to behave more like ARGUMENT. This might have far reaching consequences... so I urge you to run all test cases (which is too much for my laptop) to ensure this doesn't inadvertently reveal bugs elsewhere. An alternative solution could be to find out why the replacement IfTrees have negative pos. PS: the expected golden is slightly different than in your test case. The whitespace makes sense if you think about it.
Update on status, please?
Reassigning all moonko's java/source bugs to myself.
Any update on this Honzo? Thanks!
Hm, the patch which directly modifies the MEMBER metric seems to break too many codegen tests. I tried to introduce new metric, STATEMENT, used in diffBlock. The codegen tests pass with this metric, but still several java.hints tests fails. In some cases, the new behavior is probably OK (and so the golden output should be changed, e.g. IntroduceHintTest). In at least one case, however, the output is incorrect - a comment is missing in the output (AddParameterOrLocalFixTest.testAddLocalVariableWithComments). One of my working patches to issue #170213 was solving a similar case - it conflicted with some java.hints test too, IIRC.
OK, so do you propose Honzo that Sam modifies the patch and ask for another review then?
Any update of this issue? Can I give a try to fix it?
Yes, I think you can. Just make sure to address Jan Lahoda's comments.
by all means vezzoni, I am no longer actively contributing to Netbeans – I hope you appreciate the difficulty of this particular bug ;-)
ok Jika! fommil, it's a pity you are no longer contributing with netbeans. I really hope that you come back in the future. how are you doing Jan? I would like to NetFIX this issue, is it possible?
Created attachment 124917 [details] Diff file with test case and proposed fix Affected files on Java.Source project: * org.netbeans.api.java.source.gen.IfTest #test158154OneIf() #test158154TwoIfs() #test158154SeveralIfs() * org.netbeans.modules.java.source.save.CasualDiff:2893 #diffList( List<? extends JCTree>, List<? extends JCTree>, int, PositionEstimator, Comparator<JCTree>, VeryPretty)
Vezzoni, thanks for the patch. Are codegen tests passing with it? (cd java.source; ant -Dtest.config=generator test) Moreover, it would be good to check if java.hints tests still pass: (cd java.hints; ant test) I can run the tests, but won't likely have time for that in next week or so.
Jan, I did not run those executions. but, I did run commit-validation on java.source, as Jirka has taught me. I'm still learning these tasks. I'll run those both executions tonight (GMT-3 :o) to ensure the patch. Thank you for the tip. I'll comment here with the result.
Roberto, how did the Java Hints test run? Was it okay so that we can review your patch? Thanks for any update.
It's broken Jirka, I am still working on that. The big problem is: the IfTree (CasualDiff) is building different output statements for the same input. I can fix this behaviour, but I'll break a lot of JUnit tests. I'm stuck with that. how many time I still have to deliver this?
Created attachment 127113 [details] Proposed fix with its test case and rewriting of unit tests Proposed fix with its test case and rewriting of unit tests. the "ant -Dtest.config=generator test" at java.source was executed with success. the "ant test -Dignore.random.failures=true" at java.hints was executed with success.
Roberto, As I read the patch, it basically prints the if statements anew. I am sorry, but this is not going to be accepted - it goes against the "smallest changes possible" idea. Looking into this much deeper, the least incorrect solution may the fommil's one (it is actually enough to change MEMBER to REAL_MEMBER in diffBlock). Some java.hints tests that fail after this change actually produce better code in the updated version, but some don't produce necessarily better variant, e.g. MagicSurroundWithTryCatchFixTest.test204165b. But that may be acceptable if that happens only in rare unrealistic cases.
Ok Jan! Understood. Can I keep going through this bug?
Created attachment 132706 [details] test case and proposed fix
Honzo, can you please evaluate this?
(In reply to vezzoni from comment #29) > Created attachment 132706 [details] > test case and proposed fix I evaluated the fix and changed it a little; the fix works OK for replacing several consecutive statements of the same type. However if the last statement from the replaced range was of a different kind, the fix would fail to recognize the DELETE-INSERT pair as a replacement. The basic idea remains - thanks for providing the patch !
Fixed in changeset http://hg.netbeans.org/jet-main/rev/1aa96c213370
Oops. Wrong changeset. Should be http://hg.netbeans.org/jet-main/rev/7214900c79dd
(In reply to Svata Dedic from comment #31) Thank you very much for your time for this evaluation, Svata! I'm glad I was helpful.