Community
Participate
Working Groups
SelectionJavadocParser reports problems since this class has been created although it seems not to be really necessary to do so. Not sure if this would increase performance but I guess this could help a little bit...
Created attachment 73534 [details] [proposed patch]
(In reply to comment #1) Fix + test case - all jdt.core tests ok
Created attachment 77115 [details] same patch synchronized with HEAD v808
(In reply to comment #3) Frederic, Jerome, David: please review proposed patch
(In reply to comment #4) > (In reply to comment #3) > Frederic, Jerome, David: please review proposed patch > OK for me. Perhaps David may advice to add some tests for this?
Patch looks good to me. I agree that it needs regression tests before being committed.
(In reply to comment #6) David: requesting your assistance to elaborate test cases. Thanks.
Created attachment 78276 [details] [proposed patch + test case] on top v811 - all jdt.core tests OK improved fix + test case (David: thanks for your help on this one) May you please review? Thanks.
(In reply to comment #8) I would use a toString() representation of the problems instead of using assertFalse(res.hasProblems()). This allows a better debugging if a problem is introduced. You might want to refactor org.eclipse.jdt.core.tests.compiler.regression.Requestor#acceptResult(...) so as to extract the problemLog computing.
Created attachment 78959 [details] [proposed patch + test case] on top v813 - all jdt.core tests OK (In reply to comment #9) new test case as suggested - please review - thanks.
Created attachment 79054 [details] [proposed patch + test case] on top v814 - all jdt.core tests OK refactor Requestor#acceptResult(CompilationResult) as suggested in comment #9
(In reply to comment #11) This patch looks good to me.
(In reply to comment #11) > Created an attachment (id=79054) [details] > [proposed patch + test case] on top v814 - all jdt.core tests OK > > refactor Requestor#acceptResult(CompilationResult) as suggested in comment #9 > This patch changes the behavior for DocCommentParser while reporting problems. The default is now true (as shouldReportProblems is initialized to true) although it's currently false (as sourceParser == null). I would also prefer not to have a method to set reportProblem on JavadocParser. I think its content should be inlined systematically to minimize performances impact of this change. Finally, looking at this, I realized that, in my previous review, I missed the fact that shouldReportProblems was specific to javadoc parsers and did not concern DocCommentParser... So, perhaps this field should be implemented in JavadocParser instead.
Created attachment 79197 [details] [proposed patch + test case] on top v815 - all jdt.core tests OK as per comment #13 remarks / suggestion (well done Frederic) Also, new method int the JavadocParser was removed: inlined (duplicate) code was preferred for performance reasons. May reviewers please review proposed patch? Thanks.
Created attachment 79265 [details] [proposed patch + test case] on top v815 - all jdt.core tests OK no modification in jdt.core Modified tests.compiler as per Frederic buddy check
Created attachment 79366 [details] [proposed patch + test case] on top v815 - all jdt.core tests OK final version
(In reply to comment #16) > Created an attachment (id=79366) [details] > [proposed patch + test case] on top v815 - all jdt.core tests OK > > final version > Patch looks good: +1
(In reply to comment #16) Patch still looks good to me.
Released for 3.4M3 in HEAD stream
Verified for 3.4M3 using I20071029-0010 build.
