Community
Participate
Working Groups
Build ID: M20090826-1100 Steps To Reproduce: Formatting the below code ####### public class test1 { /** *<pre> *void foo() { *} *</pre> */ void foo() { } } ##### becomes ####### public class test1 { /** *<pre> * oid foo() { * *</pre> */ void foo() { } } ######### Note the 'v' in the void and the '{' have gone. More information: This is even reproducible in 3.6M1 and not reproducible in 3.5
I reproduced with 3.5.1RC2. Reassigning to Frédéric as I would like a fix for next week.
It looks like the character that follows the '*' is removed. If a space is inserted between 'void' and the '*', it works. Same for the '}' and the '*'.
A fix is needed for 3.5.1.
Olivier, This is not a regression, I reproduce the same behavior using 3.5.0, 3.4.2 and 3.3.2. Hence, this is even not a regression introduced while rewriting the comment formatter, so I guess this issue exists since day 1... IMO, you may reset the target (at least set to 3.5.2), reduce the severity to normal and put back this bug to Satyam to let him investigate and learn around this code.
Agreed. In this case, we can fix it only for 3.6. Targetting 3.6M2. I still leave it as major as there is data loss in the process. Reassigning to Satyam so he can learn how to fix issues in the comment formatter.
My mistake in reproducing - the problem is reproducible in 3.5 too.
Created attachment 146280 [details] Proposed patch Frederic, Can you review the changes? Thanks, Satyam
(In reply to comment #7) > Created an attachment (id=146280) [details] > Proposed patch > > Frederic, > Can you review the changes? > > Thanks, > Satyam The changes look good but need some minor improvements: 1 - Tests are usually put in the FormatterCommentsBugsTest test suite. JavaDocTestCase was used to test the comment formatter before it was rewritten in 3.4). It's not wrong to write a test there but it's easier to have all tests at the same place... 2 - change in Scribe.printCodeSnippet(int,int) should be improved: a - do not call a method twice when its result can be put in local variable b - not only ' ' and '\t' are space characters c - use a switch as soon as there are several comparisons to constants Then the code would become: int offsetEnd = prefixOffset + 1; char ch = inputBuffer.charAt(offsetEnd); switch (ch) { case '\u000c' : /* FORM FEED */ case ' ': case '\t': offsetEnd++; break; default: if (ScannerHelper.isWhitespace(ch)) { offsetEnd++; } break; } inputBuffer.delete(lineOffset, offsetEnd); 3 - in printJavadocHtmlTag(FormatJavadocText, FormatJavadocBlock, boolean), the change from nextStart to realEnd as a side effect on existing behavior. I observed it while running the FormatterCommentsMassiveTests test suite on a Galileo full source workspace. Undoing this change makes the behavior b and with your patch I see a lot of difference vs. the output produced by HEAD... And I strongly suspect that this is
Sorry, a unexpected hit on the Enter key before I actually finish my comment... So, point 3 should be read: 3 - Can you explain a little bit why you change nextStart to realEnd? At least by specific test showing that it was necessary. 4 - The patch has side effects on existing comment formatter behavior. I observed it while running the FormatterCommentsMassiveTests test suite on a Galileo full source workspace. You need to understand first if these side effects are acceptable (i.e. fix incorrect previous behaviors) or if they are regressions...
There's another issue with your patch, it does not fix the following test case: public class X { /** * <pre> {@code * * public class X { * }}</pre> */ void foo() {} } which is both formatted with or without your patch as: public class X02b { /** * <pre> * @code * * public class X { * }} * </pre> */ void foo() { } } The brace of the inline tag @code still disappears when inserting the new line after the <pre> html tag...
Moving to 3.6M3 as this won't be part of the next I-build.
Frederic, Thank you for your comments. 1. I would move the test accordingly. 2. I completely agree for your comment of the duplicate call. I would fix it. 3. The main reason we are running into this problem is because we are setting the nextStart to '*'position+1, even if it running into the next separator. Hence, I am trying to use one more variable realEnd so that we make sure that we add '1' for those that are really having atleast one space. 4. Can you tell me how to run the FormatterCommentsMassiveTests test suite? 5. I feel that the '{' disappearance problem is slightly different and probably better handled as part of another bug. Do you think otherwise?
(In reply to comment #12) > Frederic, Thank you for your comments. > > 1. I would move the test accordingly. > > 2. I completely agree for your comment of the duplicate call. I would fix it. > > 3. The main reason we are running into this problem is because we are setting > the nextStart to '*'position+1, even if it running into the next separator. > Hence, I am trying to use one more variable realEnd so that we make sure that > we add '1' for those that are really having atleast one space. > OK, but I do not think this new variable is necessary, the spaceFound flag sounds to be enough for me. If you did find some cases where it's not true, then you should add specific tests to highlight them... > 4. Can you tell me how to run the FormatterCommentsMassiveTests test suite? > Of course. The FormatterCommentsBugsTest test suite is in the org.eclipse.jdt.core.tests.formatter package. Create a launch configuration for this test suite and add the following VM arguments: -Xmx256M -DinputDir=<source directory> -DoutputDir=<output directory> When running this test suite, the source directory is parsed recursively to find all the java files, format each of them twice and write the output result in the output directory. Typically, the source directory may be a workspace where you import the sources of all the target platform plugins, eg. Import > Plug-ins and Fragments > Projects with source folders. Personally, I import sources of all Galileo plugins and get more than 41,000 java units to format... So, the process to test your patch is: 1 - create and fill the source directory 2 - run the FormatterCommentsBugsTest test suite against this workspace using HEAD contents; this will create the outputs which will be used as reference 3 - repeat step 2 and store the console output in a text file 4 - apply your patch 5 - run the FormatterCommentsBugsTest test suite against this workspace 6 - compare the console output with the one stored in step 3, only date and time should differ. If there are differences, then they must be analyzed to understand whether there are justified (incorrect behavior fixed by the patch) or regressions... > 5. I feel that the '{' disappearance problem is slightly different and probably > better handled as part of another bug. Do you think otherwise? > I really don't know. If you think it's a separated issue then open a specific bug for it looks the good thing to do.
I have analyzed the failures. It is just that the blank lines in the pre tag were not getting deleted with this patch. Fixing them!
Created attachment 147524 [details] Proposed patch Apart from your earlier comments, I have added two more case statements for \r and \n in the switch statement. I have also added two more test cases to make sure that we catch those changes observed in the massive tests.
(In reply to comment #15) > Created an attachment (id=147524) [details] > Proposed patch > > Apart from your earlier comments, I have added two more case statements for \r > and \n in the switch statement. I have also added two more test cases to make > sure that we catch those changes observed in the massive tests. This patch sounds OK, however I'm still annoyed by the local variable realEnd. It's not clear why you need an extra variable instead of only nextStart. So, I've try to rewrite entirely this part of the algorithm to make it clearer. I'll attach it to this bug then we can chose the one appearing to be the best...
Created attachment 147536 [details] Other proposed patch The first thing it appears to me is that rescan the text was not necessary as we already knows the number of empty lines (linesGap), hence it's only necessary to scan the beginning of the line before the code start (nextStart) to see whether a space is needed or not...
This patch looks much better. I believe this could be final!
Since there is data loss, please investigate a patch for 3.5.2. Thanks.
(In reply to comment #18) > This patch looks much better. I believe this could be final! Thanks Satyam. So, as I'm back to JDT/Core team and keep the formatter stuff in my area, I take this bug back to me... I really appreciated your helpful contribution :-)
Comment on attachment 147524 [details] Proposed patch mark this patch as obsolete as we agreed to take the other patch instead
Released for 3.6M3 in HEAD stream. (In reply to comment #19) > Since there is data loss, please investigate a patch for 3.5.2. > Thanks. => set the target to 3.5.2. I do not think there will be any problem to backport it to the R3_5_maintenance stream...
Created attachment 147885 [details] Patch for R3_5_maintenance stream
Olivier, could you please review?
Patch looks good.
This seems to be released in the 3.5 maintenance stream. Please close as RESOLVED/FIXED.
(In reply to comment #26) > This seems to be released in the 3.5 maintenance stream. > Please close as RESOLVED/FIXED. Ooops that's right, sorry I forgot to update the bug... Released for 3.5.2 in R3_5_maintenance stream.
Frédéric, please update the bundle version to 3.5.2.
(In reply to comment #28) > Frédéric, please update the bundle version to 3.5.2. done
Verified for 3.6M3 using I20091027-0100.
Verified for 3.5.2RC2 using M20100120-0800. Opened bug 300379 for problem reported in comment 10.
*** Bug 303957 has been marked as a duplicate of this bug. ***
