Skip to content
This repository was archived by the owner on Dec 19, 2018. It is now read-only.

Use the new line character to check if the CodeWriter buffer ends in a line feed. #616

Closed
wants to merge 2 commits into from

Conversation

pranavkm
Copy link
Contributor

Fixes #577

@@ -114,7 +114,7 @@ public void Dispose()
{
// Need to add an additional line at the end IF there wasn't one already written.
// This is needed to work with the C# editor's handling of #line ...
var endsWithNewline = _writer.GenerateCode().EndsWith("\n");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be buggy on CoreCLR \ Linux. https://github.com/dotnet/corefx/issues/4587

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect you can work around issue (and have better performance) by explicitly passing StringComparison.Ordinal to EndsWith.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine either way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible for the Builder to be empty at this point? If that's even a remote possibility, ...Builder.EndsWith("\n", StringComparison.Ordinal) sounds better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StringBuilder doesn't have an EndsWith method. I think @ellismg was referring to the string we were producing. I added a bounds check for the char check.

@Eilon
Copy link
Contributor

Eilon commented Nov 19, 2015

:shipit: either way 😄

@@ -18,6 +19,8 @@ public class CodeWriter : IDisposable
private int _currentLineIndex;
private int _currentLineCharacterIndex;

public StringBuilder Builder => _writer.GetStringBuilder();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Could CSharpLineMappingWriter use the existing LastWrite property instead? (Glad you've moved away from the weird writer -> string implicit conversion.)
  2. If not, does the new property need to be in CodeWriter? CSharpLineMappingWriter knows it has a CSharpCodeWriter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. There's a bunch of places we do GenerateCode().Length. It would be a lot nicer to simply do Buffer.Length instead. I'll send that as a follow up.
  2. Don't see why it's better to move it a derived type.

@dougbu
Copy link
Contributor

dougbu commented Nov 19, 2015

@pranavkm
Copy link
Contributor Author

🆙 📅

@NTaylorMullen
Copy link

:shipit:

1 similar comment
@dougbu
Copy link
Contributor

dougbu commented Nov 19, 2015

:shipit:

@pranavkm pranavkm closed this Nov 19, 2015
@pranavkm pranavkm deleted the prkrishn/577 branch November 19, 2015 23:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants