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
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2b3315e
Use the new line character to check if the CodeWriter buffer ends in …
pranavkm Nov 19, 2015
cbf5e78
Changes per PR comments
pranavkm Nov 19, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Microsoft.AspNet.Razor/CodeGenerators/CSharpLineMappingWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
var endsWithNewline = _writer.Builder[_writer.Builder.Length - 1] == '\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.


// Always write at least 1 empty line to potentially separate code from pragmas.
_writer.WriteLine();
Expand Down
3 changes: 3 additions & 0 deletions src/Microsoft.AspNet.Razor/CodeGenerators/CodeWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.IO;
using System.Text;

namespace Microsoft.AspNet.Razor.CodeGenerators
{
Expand All @@ -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.


public string LastWrite { get; private set; }

public int CurrentIndent { get; private set; }
Expand Down