Skip to content

Conversation

@OmarTawfik
Copy link
Contributor

/cc @Microsoft/fsharp-compiler @jasonmalinowski @CyrusNajmabadi @dsyme


interface IBraceMatcher with
member this.FindBracesAsync(document: Document, position: int, cancellationToken: CancellationToken): Task<Nullable<BraceMatchingResult>> =
let computation = async {
Copy link
Member

Choose a reason for hiding this comment

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

will this do an implicit ConfigureAwait(false)? If not, can you change this to: document.GetTextAsync(ct).ConfigureAwait(false)

Copy link
Member

Choose a reason for hiding this comment

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

Curious: is there a correctness reason for this or is it just to get better perf?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@dsyme
Copy link
Contributor

dsyme commented Mar 1, 2016

Great work.

open Microsoft.FSharp.Compiler.SourceCodeServices

// TODO: add types colorization if available from intellisense
// TODO: add defines flags if available from project sites and files
Copy link
Member

Choose a reason for hiding this comment

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

Can we file bugs for each of these and reference them here instead of TODO? A TODO is easy to forget about, bugs less so.

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 is being added in the next PR, as it was the best way to break down the large intellisense changes into separate PRs.

@OmarTawfik
Copy link
Contributor Author

Addressed comments.
@jaredpar @dsyme can you please comment on the open comments?

@KevinRansom
Copy link
Contributor

I like this, nice work.


[<TestCase("[start")>]
[<TestCase("]end")>]
member this.BraceInMultiLineCommentShouldnotBeMatched(startMarker: string) =
Copy link

Choose a reason for hiding this comment

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

Minor point: ShouldNot here and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

right <- middle - 1
else
result <- Some(middleToken)
result
Copy link

Choose a reason for hiding this comment

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

Consider implementing recursively.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@cloudRoutine
Copy link
Contributor

Shouldn't [< >], [| |], <@ @> <@@ @@> 'brace pairs' be included too?

endPosition <- endPosition + 1
let textSpan = new TextSpan(textLine.Start + startPosition, endPosition - startPosition)
result.Add(new ClassifiedSpan(classificationType, textSpan))
startPosition <- endPosition
Copy link

Choose a reason for hiding this comment

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

It seems unnecessary to convert from tokens to a character-based colorMap back to token-based ClassifiedSpans. Can we avoid colorMap and map directly from tokens to ClassifiedSpans?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The line scanner produces more tokens that can be grouped. This one-time optimization (borrowed from the deprecated code) provides performance optimization to Roslyn.

@OmarTawfik
Copy link
Contributor Author

Modifying the cache to hold lex states for each line, to allow partial parsing per @dsyme comments.

OmarTawfik pushed a commit that referenced this pull request Mar 4, 2016
Added Colorization/Brace Matching Roslyn Services
@OmarTawfik OmarTawfik merged commit 7744f53 into dotnet:roslyn Mar 4, 2016
@OmarTawfik OmarTawfik deleted the correcthistory branch March 4, 2016 22:54
@dsyme
Copy link
Contributor

dsyme commented Mar 5, 2016

@OmarTawfik
Copy link
Contributor Author

@dsyme it is because I'm building against a non-shipped version of Roslyn (on myget), which has internals exposed to F#, so the nuget packages expire quickly. Later on when we have a more stable nuget package, I'll switch this F# branch to use it.

@enricosada
Copy link
Contributor

@otawfik-ms can you add the myget feed in Nuget.config? that should fix appveyor, and local build too.
If this project is really open source, all dependancies should exists outside microsoft (binaries or sources), otherwise it's not open source, it's a code dump /cc @KevinRansom @NumberByColors

@OmarTawfik
Copy link
Contributor Author

@enricosada myget is out and public :) Roslyn uses it for daily builds, and it is added to this branch's nuget config:
https://github.com/Microsoft/visualfsharp/blob/roslyn/.nuget/NuGet.Config
The issue is that we are still depending on the daily builds on Roslyn, and they expire quickly (myget deletes older packages, nuget holds them forever).
Devs working on this branch need to consistently update their packages.config with newer versions, until Roslyn merges these changes into a stable branch and produce a permanent nuget package for us to use.

@CyrusNajmabadi
Copy link
Member

Looks ok to me.

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.