Summary: | [javadoc][assist] SelectionJavadocParser should not report problems | ||
---|---|---|---|
Product: | [Eclipse Project] JDT | Reporter: | Frederic Fusier <frederic_fusier> |
Component: | Core | Assignee: | Eric Jodet <eric_jodet> |
Status: | VERIFIED FIXED | QA Contact: | |
Severity: | normal | ||
Priority: | P3 | CC: | david_audel, frederic_fusier, jerome_lanneluc |
Version: | 3.3 | ||
Target Milestone: | 3.4 M3 | ||
Hardware: | PC | ||
OS: | Windows XP | ||
Whiteboard: | |||
Attachments: |
Description
Frederic Fusier
2007-06-13 10:08:27 EDT
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. |