Bug 493296 - Eclipse formatter hangs when formatting with formatter.join_wrapped_lines=true
Summary: Eclipse formatter hangs when formatting with formatter.join_wrapped_lines=true
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 4.5.2   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 4.7 M3   Edit
Assignee: Mateusz Matela CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-10 04:39 EDT by Nicole Rauch CLA
Modified: 2016-10-26 05:48 EDT (History)
5 users (show)

See Also:
manoj.palat: review+


Attachments
The file that the formatter gets stuck on. (81.46 KB, application/octet-stream)
2016-05-10 04:39 EDT, Nicole Rauch CLA
no flags Details
The full formatter settings. (23.44 KB, application/octet-stream)
2016-05-10 04:42 EDT, Nicole Rauch CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole Rauch CLA 2016-05-10 04:39:25 EDT
Created attachment 261581 [details]
The file that the formatter gets stuck on.

When trying to format the attached file with the Eclipse formatter while the setting formatter.join_wrapped_lines=true is set, the formatter hangs.
Comment 1 Nicole Rauch CLA 2016-05-10 04:42:13 EDT
Created attachment 261582 [details]
The full formatter settings.
Comment 2 Eclipse Genie CLA 2016-09-21 18:02:09 EDT
New Gerrit change created: https://git.eclipse.org/r/81644
Comment 3 Mateusz Matela CLA 2016-09-21 18:32:38 EDT
(In reply to Eclipse Genie from comment #2)
> New Gerrit change created: https://git.eclipse.org/r/81644

I had to change a lot in the line wrapping implementation, mainly drop recursion and use my own data structures as processing stack. With that, I managed to get rid of the exception throwing mechanism. Also with some aggressive optimizations, large files should be now a lot less problematic.

Using the occasion, I did some refactoring of WrapMode enum and surrounding code: new value FORCE for "Force wrap" policies, renamed FORCED to BLOCK_INDENT (I don't know why the old name ever seemed reasonable to me).

Manoj, requesting a review as this is a major change. There may be some new bugs, bug in the long run the code should be easier to maintain, and there's probably a few unknown bugs fixed by this patch.

I hope the new test is sufficient. I was testing on a 32bit jvm and without the fix I just got stack overflow before experiencing any freeze.

Changes in existing tests:
- test477476: lines force-wrapped by line comments or region limits are no longer touched, even if an additional wrap inside would result in a better score. This is more consistent with the behavior for "Never join already wrapped lines". 
- testBug465669(): the algorithm is better at minimizing line width overflow when keeping within the line limit is impossible
Comment 6 Eclipse Genie CLA 2016-10-12 14:10:19 EDT
New Gerrit change created: https://git.eclipse.org/r/83054
Comment 7 Mateusz Matela CLA 2016-10-12 14:15:12 EDT
(In reply to Dani Megert from comment #5)
> This causes a test failure in JDT UI:

The test result has changed as per comment 3:
> lines force-wrapped by line comments or region limits are no longer touched,
> even if an additional wrap inside would result in a better score. This is
> more consistent with the behavior for "Never join already wrapped lines".

(In reply to Eclipse Genie from comment #6)
> New Gerrit change created: https://git.eclipse.org/r/83054
The affected lines now go back to the state they were before bug 303519.
Comment 9 Dani Megert CLA 2016-10-12 15:23:29 EDT
(In reply to Mateusz Matela from comment #7)
> (In reply to Dani Megert from comment #5)
> > This causes a test failure in JDT UI:
> 
> The test result has changed as per comment 3:
> > lines force-wrapped by line comments or region limits are no longer touched,
> > even if an additional wrap inside would result in a better score. This is
> > more consistent with the behavior for "Never join already wrapped lines".
> 
> (In reply to Eclipse Genie from comment #6)
> > New Gerrit change created: https://git.eclipse.org/r/83054
> The affected lines now go back to the state they were before bug 303519.

Thanks, makes sense. I've reviewed and merged the change.
Comment 10 Jay Arthanareeswaran CLA 2016-10-26 05:48:40 EDT
Verified for 4.7 M3 with build I20161024-2000