Scanner Peep.

[AdamSpeight2008]

[Compiler VB Scanner] Implement Peep and usage within the scanner.

• ScannerXML
• Scanner
• ScannerInterpolatedString

Peep is a integration of the contents of CanGet and Peek as these often appear in close proximity. By combining them the source code is shorter, and potentially slightly quicker as the 'ch` value can be cached, and thus reused later. Reducing repetition of multiple function calls.
(On my machine time to run the tests reduced from 21mins to 20mins)

Note: Peep is similar to a TryParse function but it implements different semantics in the false case, where it preserves the existing value contain in ch

There may be some potential reduction in the number of temporary char variables, with closer examination, but this can be left for another PR.

[AdamSpeight2008]

I'm doing a few refinements of the temporary char variables I've introduced, see which I remove (reuse an existing char variable.

[davkean]

tag @dotnet/roslyn-compiler

[AdamSpeight2008]

Test time down to around 18-19 minutes. (On my machine)

[AdamSpeight2008]

Ready for review.

[TyOverby]

Why is this one using ch and the others us cx?

[AdamSpeight2008]

@TyOverby
I'll re-examine this method.
Some of the case can alter the value of ch as all code path in that case exit from the function. It is the case's that don't I have to be careful with as ch is used after the Select block.

[AdamSpeight2008]

@dotnet-bot retest this please.

[AdamSpeight2008]

Re-examined the usage of Peep and reduced the number of temporary char variables.

[gafter]

@dotnet/roslyn-compiler Anybody care to comment on this? @AlekseyTs ?

[AdamSpeight2008]

@AlekseyTs I've also got a branch that integrate this into the future branch.

[AdamSpeight2008]

@gafter Ok, I'll do that and rerun the tests. Maybe a couple of hours before the results are posted.

[AdamSpeight2008]

@gafter
Here are the results of the testing.

Original

Run #1  00:11:38.8527845
Run #2  00:14:07.7066477
Run #3  00:12:34.9365867
        ----------------
        00:38:21.4960189 sum
        ----------------
        00:12:40.4986726 avg


Proposed

Run #1  00:11:22.0387645
Run #2  00:13:49.6298996
Run #3  00:11:57.7907524
        ----------------
        00:36:29.4694165 sum
        ----------------
        00:12:09.8231388 avg

I think I've calculated those totals and averages correctly. ❓

Zipped Files

[gafter]

This looks good to me. @dotnet/roslyn-compiler Can I please have another set of eyes on this?

[TyOverby]

Someone at lunch mentioned that having "peek" and "peep" intermingled could confuse readers. Fortunately we have a naming convention for functions that return bool for success/failure and use out parameters.

What about changing "peep" to "tryPeek"?

[AdamSpeight2008]

@TyOverby
I've delibratly not used Try as it doesn't follow the same semantics of a Try.

     Private Function Peep(skip As Integer, ByRef ch As Char) As Boolean

Notice the ByRef ch As Char doesn't have an <Out> attribute, This behaviour is intentional.

If Peep doesn't succeed, it maintains and doesn't overwrite the value currently contained in the out parameter. Whereas a "Try" typically returns default(T) for out parameter. I didn't want Peep to change, as where it begin used, Typically within the condition of an If statement, the "out" value is sometimes use in the Else / ElseIf branch. By returning the preexisting value, the code within those branches are left unaltered in there potential actions.

Method naming was discussed previously and was suggested that a distinction be made. I also added the XML comments also, so that this information was available in intensense. I didn't want the name to be too long as it is used a lot in the scanner code. A previous PR (#1627) shortened a lot of these common methods, which made the source a lot simple to read.

• Peek and Peep are related
• Peep is CanGet + Peek (combined)
• Peep is the only one of these method that passes back a value, via it's input parameters.
• Changing a Peek into Peep is just one letter, then you get intelisense errors.
• This PR is also about reducing the size of the scanner sourcecode. I find it a lot easier to look through than the existing.

@gafter Let me just re run the tests as it looks like the proposal has the wrong dll. The one without Peep?

[AdamSpeight2008]

@gafter After rerun the test, with the correct DLL (with Peep) for the Proposal

Run #1  00:11:56.5150290
Run #2  00:10:02.4786336
Run #3  00:12:08.7661797
        ----------------
        00:34:07.7598426 sum
        ----------------
        00:11:35.9199475 avg

Zipped Files

[gafter]

@AdamSpeight2008 Can you please give an example of where, in your code, you depend on Peep not modifying the ref parameter when it returns False? I would expect it to be more clear to use the "try" pattern and just use separate variables, no?

• Peek and TryPeek would be even more clearly related than Peek and Peep
• the name itself would clearly designate its intent (no intellisense needed)
• Reducing the size of the codebase (e.g. by shortening identifiers) is a non-goal.
• Clarity of source (which is an important goal) is enhanced by using common coding idioms.

[AdamSpeight2008]

@gafter
Any place where is use in the form where it is the First argument in a short-circuiting condition.

' ch as some existing value before here
While Peep(Here, ch) AndAlso IsFoo(ch)
...
End While 
Dim bar = Foo (ch)

Imagine the While is false because of the Peep on the first iteration. The Foo(ch) would potentially the wrong value.

Source

        Private Function ScanLineContinuation(tList As SyntaxListBuilder) As Boolean
            Dim ch As Char
            If Not Peep(0, ch) OrElse
               Not IsAfterWhitespace() OrElse
               Not IsUnderscore(ch) Then
                Return False
            End If

Let say Peep(ch,0) returns True.

            Dim Here = 1
            While Peep(Here, ch) AndAlso IsWhitespace(ch)
                Here += 1
            End While

This one Peep( 1, ch ) return false

            ' Line continuation is valid at the end of the
            ' line or at the end of file only.
            Dim atNewLine = IsNewLine(ch)

IsNewLine(ch) potentially passed the wrong value.

[AdamSpeight2008]

Another Example Possible Assert Failure
Another Example Possible Assert Failure
Maybe some more case.

[gafter]

That is way, way too subtle. I'm afraid this code is far too delicate if that is your intent. It is likely to break when next it is maintained. I'd prefer to see that code changed from IsNewLine(ch) to Peek(Here, ch) AndAlso IsNewLine(ch). In that case the move to the "try" pattern would be possible. At least in the original code this was all explicit.

Perhaps better to only use the new Peep/TryPeek in cases where the try pattern works, and leave the code the old way where it does not?

[AdamSpeight2008]

@gafter Don't blame me for casing, I just working with what already existed.

[AdamSpeight2008]

@gafter

That is way, too subtle.

It maybe so but they are pre-existing within the scanner source, the PR doesn't change that. It just exposes it because of the PR touch that area of the source.

At least in the original code this was all explicit.

It still is explicit, just that Peep can exploit the fact that CanGet and Peep are used in closed proximity, so there is (as the test show) potential for efficiencies. The original code also involved re-lookingup things that where previously used. Peep allows us to cache that, whilst not having separate it out into a separate section of code.

in cases where the try pattern works, and leave the code the old way where it does not?

CanGet( Here ) AndAlso Peek(here) = foo is a try pattern.

All of the usage can be replace with a Peep, I reverted some back to original forms, as the code was simpler.

[gafter]

I think the code would be much more clear if this kind of method were only used when the "try" pattern were applicable. A method that takes a ref parameter and sometimes assigns it and sometimes does not, and a caller that takes advantage of the difference, is just way too subtle. That was not a pre-existing pattern in the code. That would also enable you to rename the method so it follows a familar "try" coding pattern.

[jaredpar]

That is way, way too subtle

Completely agree. I've looked at this PR a few times and never noticed that.

[AdamSpeight2008]

@jaredpar and @gafter
Reexamine ScanLineContinuation in the existing master, you will see this subtle behavhiour in that method.

First one succeeds, get to While. It fails on CanGet (1st pass). What do the potential two usages of ch expect to see?

[gafter]

The original code was clear because the assignments were explicit, under conditional statements.

[AdamSpeight2008]

It still is explicit under a conditional statement, only that they've been combined into a single function.
The function maintains existing semantics of the original code.

@Pilchie Any comment?.

[gafter]

Though we appreciate your efforts, we do not want to accept this change as written, though we might consider a change that uses the try pattern where appropriate.

To be clear...

CanGet( Here ) AndAlso Peek(here) = foo is a try pattern.

No, that is not a try pattern. That is code that could be rewritten into the try pattern. A try pattern is a method (in C# syntax)
bool TrySomething(..., out SomeType result) ...
used in an idiomatic way
if (TrySomething(..., out result)) { /* code using result */ }

The reasons we do not want to pull these changes are

• The code does not fix any bug in Roslyn
• The code does not address any identified performance issues
• The code uses non-idiomatic coding patterns that are likely to result in support issues later
• This is one of the most delicate areas in the compiler, and we are reluctant to change it unless we have a very good reason to do so.

If you're interested in contributing to the Roslyn compilers, I recommend you select issues that we've already identified as things we'd like fixed and would welcome community contributions. For example, #2150 is low-hanging fruit.

[AdamSpeight2008]

It would perform worse if it followed the other try patterns, as it would involve an extra assignment on each false.

[AdamSpeight2008]

There are functions with the current source that have the "Try" prefix,return a boolean and don't have an <out> parameter on the pass-back parameter,

Block of Four in Parser.vb