Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing addNewLine #5941

Closed
wants to merge 4 commits into from
Closed

Removing addNewLine #5941

wants to merge 4 commits into from

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented Nov 20, 2018

This removes addNewLine. It appended an entire source string with "\n" causing massive allocations for large source files.

@auduchinok
Copy link
Member

Fixes #3617.
I can confirm everything works as expected when no new line is added except for tests need fixing.

@cartermp
Copy link
Contributor

@TIHan TIHan#4

@cartermp
Copy link
Contributor

Some of the other tests here are harder to fix...

@cartermp
Copy link
Contributor

Sweet, only 82 tests left. They're in the legacy langauge service tests, making it extra fun!

* Update interactivechecker and structure tests

* Try to fix some legacy tests
@cartermp
Copy link
Contributor

Okay, so this change definitely breaks completion. Everything else seems fine, so there's something about completion that requires a newline at the end of the file...

@cartermp
Copy link
Contributor

The "break" is actually just a breaking change from a newer API that was then removed in dev16. Unrelated to this PR.

* Update interactivechecker and structure tests

* Try to fix some legacy tests

* More test fixes
@cartermp
Copy link
Contributor

Blegh it won't be as easy as I thought then. These old tests are horribly complex 😢

@cartermp
Copy link
Contributor

cartermp commented Nov 30, 2018

Just posting this for my amusement before going to bed.

FCS says we need a newline because of tests:
https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/service/service.fs#L1548

Tests say we need to do something because FCS adds a new line:
https://github.com/Microsoft/visualfsharp/blob/master/vsintegration/tests/UnitTests/LegacyLanguageService/Tests.LanguageService.ParameterInfo.fs#L972

Tests also say that FCS adds three new lines, hence a (5, 0), but there is no (5, 0), and FCS doesn't add three new lines:
https://github.com/Microsoft/visualfsharp/blob/master/vsintegration/tests/UnitTests/LegacyLanguageService/Tests.LanguageService.ParameterInfo.fs#L885

So FCS does something because of tests, and tests do something because of FCS.

image

But what's actually bothering me the most is this:

(testLines.Length-1)+1

Time for bed.

@TIHan
Copy link
Contributor Author

TIHan commented Dec 14, 2018

Closing as this PR resolves it: #6001

@TIHan TIHan closed this Dec 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants