Community
Participate
Working Groups
Using 3.4RC1 candidate (I20080515-2000). Format the following test case: /** * {@link SearchEngine#searchAllTypeNames( * char[] packageName, * int packageMatchRule, * char[] typeName, * int typeMatchRule, * int searchFor, * IJavaSearchScope scope, * TypeNameMatchRequestor nameMatchRequestor, * int waitingPolicy, * org.eclipse.core.runtime.IProgressMonitor monitor)} */ public class X01 { } The output is: /** * {@link SearchEngine#searchAllTypeNames(char[]packageName, intpackageMatchRule, char[]typeName, inttypeMatchRule, intsearchFor, IJavaSearchScopescope, TypeNameMatchRequestornameMatchRequestor, intwaitingPolicy, org.eclipse.core.runtime.IProgressMonitormonitor)} */ public class X01 { } There's only one line (it's bugzilla which splits it...), but first note that there's no space between arguments type and name. If I try to reformat this output again, then I get: /** * {@link SearchEngine#searchAllTypeNames(char[]packageName, * intpackageMatchRule, char[]typeName, inttypeMatchRule, intsearchFor, * IJavaSearchScopescope, TypeNameMatchRequestornameMatchRequestor, * intwaitingPolicy, org.eclipse.core.runtime.IProgressMonitormonitor)} */ public class X01 { } This time this is not bugzilla which is adding the lines, it's the formatter...
The problem comes from the method: org.eclipse.jdt.internal.formatter.Scribe.printJavadocBlockReference(FormatJavadocBlock, FormatJavadocReference) The first space problem between arguments type and name is that it should add a space only when previous token is a comma, but in fact when when the previous token is neither a '*' nor a left parenthesis. I'm investigating the second reformatting problem...
The split occurs during the second formatting because the output of the first formatting leads to a syntax error. In this case, the reference is just only considered as simple text, hence possibly split if over the line max length... So, fixing the first issue will fix both of them...
Created attachment 100624 [details] Proposed patch
Jerome, I think this will be a good candidate for RC2 as it fixes 4 failures in Ganymede workspace and it's a tiny and localized change.
Comment on attachment 100624 [details] Proposed patch This patch does not handle all cases. Typically when there are spaces inside qualified name...
One more complex example: package test.bugs.b232466; /** * {@link * test . bugs. * b232466 .X03.LocalClass03 * # * foo ( * char [ ] * packageName, * java . lang .String * packageMatchRule, * char [ ] * typeName * ) * } */ public class X03 { class LocalClass03 { void foo(char[] packageName, String str, char[] typeName) { } } } Should be formatted as: package test.bugs.b232466; /** * {@link test.bugs.b232466.X03.LocalClass03#foo(char[] packageName, java.lang.String packageMatchRule, char[] typeName)} */ public class X03 { class LocalClass03 { void foo(char[] packageName, String str, char[] typeName) { } } } But with initial proposed patch is incorrectly formatted as: package test.bugs.b232466; /** * {@link test . bugs. b232466 .X03.LocalClass03 # foo (char [ ] packageName, java . lang .String packageMatchRule, char [ ] typeName ) * } */ public class X03 { class LocalClass03 { void foo(char[] packageName, String str, char[] typeName) { } } }
Created attachment 100815 [details] Better patch but still no perfect... This patch address the complex test case of comment 6, but unfortunately introduces 5 failures in Eclipse 3.0 full src workspace => that means more work is required on this issue. So, I guess, this implies a no go for RC2 and this bug should be targeted for 3.4.1 instead!
The test case from comment 0 should at least be fixed for RC2 since it introduces an invalid reference. Patch from comment 3 is a good candidate. We should enter a new bug for the test case from comment 6.
Created attachment 101206 [details] First proposed patch on top of v_866
Maxime, David, can you please review?
(In reply to comment #8) > The test case from comment 0 should at least be fixed for RC2 since it > introduces an invalid reference. Patch from comment 3 is a good candidate. We > should enter a new bug for the test case from comment 6. > I've opened bug 233167 for the comment 6 issue.
Patch looks good: +1.
I believe that the following code is equivalent but slightly faster than the one that is proposed (especially since -1 is hopefully not the most frequent value - I computed no stats though): switch (previousToken) { case TerminalTokens.TokenNameMULTIPLY : case TerminalTokens.TokenNameLPAREN: break; default: // space between method arguments spacePosition = buffer.length(); // deliberate fall-through case -1: buffer.append(' '); this.column++; // space before reference break; }
Created attachment 101239 [details] New proposed patch after Maxime's review
David, could you please review it again? Thanks
New patch looks good and better: +1.
+1
Released for 3.4RC2 in HEAD stream.
verified for 3.4RC2 using build I20080523-0100
-
