Rename commonly used method to a shorter version. #1627
Conversation
…de readability. CanGetCharAtOffset -> CanGet PeekAheadChar -> Peek PeakChar -> Peek
gafter
added
Area-Compilers
4 - In Review
| PeekAheadChar(6) = "T"c AndAlso | ||
| PeekAheadChar(7) = "A"c AndAlso | ||
| PeekAheadChar(8) = "["c Then | ||
| If CanGet(8) AndAlso |
There was a problem hiding this comment.
Consider using NextAre(3,"CDATA[") as the condition.
|
Note To Self Thought a lot of these were implemented in #1001 |
|
I'm a little overwhelmed with the volume of comments already existing on this PR. It looks like there are enough self-reported issues that you should revise the code before we review it. |
|
@gafter Yah I self-reported as I though that I'd already implemented them, but I was think of my personal one. So I fix up those, and post and update. The other PR can be reviewed as that I ain't going go near it, till a yay or a nay. :-). |
Modified ScannerXML to use NextAre and NextIs, the remaining candidates are mostly suitable for TryPeek (not yet implemented)
Fixed the failing `Debug.Assert`s that used `AreNext(0, " ")` as the failure was being cause by CanGet looking to far ahead (off by one)
|
@dotnet-bot Test this please. |
|
I'm retesting on my machine. |
|
@gafter I've fixed the failing test. |
| If CanGetCharAtOffset(Here + 2) AndAlso _ | ||
| PeekAheadChar(Here + 1) = "]"c AndAlso _ | ||
| PeekAheadChar(Here + 2) = ">"c Then | ||
| If NextAre(Here + 2,"]>") Then |
There was a problem hiding this comment.
What is going on here? Why is this Here + 2 instead of Here + 1 ?
There was a problem hiding this comment.
Oh, I see you've already fixed this.
|
@AlekseyTs How does this look to you? |
| @@ -390,21 +390,35 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax | |||
|
|
|||
| #Region "Buffer helpers" | |||
|
|
|||
| Private Function NextAre(chars As String) As Boolean | |||
| Debug.Assert(Not String.IsNullOrEmpty(chars)) | |||
| Dim n = chars.Length-1 | |||
There was a problem hiding this comment.
The "-1" doesn't seem right to me. Consider that this should be equivalent to NextAre(0, chars)
The "-1" should be on the upper bound of the For loop on line 397
There was a problem hiding this comment.
@pharring I'll retest it with NextAre(0, chars) as this method's implementation.
There was a problem hiding this comment.
@pharring Result 6 failures, if we use AreNext(0, chars)
So this could suggest that the implementation of AreNext is incorrect.
If it is, it likely to do with this check.
If Not CanGet(offset + n) Then Return False
so I'll try
If Not CanGet(offset + n-1) Then Return False
and retest.
|
@pharring I've modified |
| Next | ||
| Return True | ||
| End Function | ||
|
|
||
| Private Function CanGetChar() As Boolean | ||
| Private Function NextIs(offset As Integer, c As String) As Boolean |
There was a problem hiding this comment.
It feels like parameter 'c' should be a Char rather than a String.
There was a problem hiding this comment.
@AlekseyTs I'll fix the method signature
|
Other than the NextIs signature, LGTM. |
Rename commonly used method to a shorter version. To aid in source code.readability.
No formatting changes.