[CSharp] Add a new CharStream that converts the symbols to upper or lower case. #2140
Conversation
|
|
||
| if (c <= 0) return c; | ||
|
|
||
| var o = Convert.ToChar(c); |
There was a problem hiding this comment.
Please use casing instead of Convert methods. It's faster.
char o = (char)c;|
|
||
| var o = Convert.ToChar(c); | ||
|
|
||
| if (upper) return Convert.ToInt32(char.ToUpper(o)); |
There was a problem hiding this comment.
Use an invariant method here: char.ToUpperInvariant(o). Lexer should not depend on users culture.
|
|
||
| if (upper) return Convert.ToInt32(char.ToUpper(o)); | ||
|
|
||
| return Convert.ToInt32(char.ToLower(o)); |
There was a problem hiding this comment.
Casting: return (int)char.ToLower(o).
| { | ||
| int c = stream.LA(i); | ||
|
|
||
| if (c <= 0) return c; |
There was a problem hiding this comment.
The one-line statement is not used in C# runtime. Please use the following style:
if (c <= 0)
{
return c;
}
|
Thanks for the review @KvanTTT. |
|
Could you also add runtime tests with chars case changing? See related PR as a sample. Also, include a test with another culture (Turkish), see this code fragment for example |
|
@KvanTTT I am not sure about which csproj the tests should be added to, as there don't seem to be any CSharp tests. I'd greatly appreciate some pointers :) |
|
Since this PR addresses a rather generic requirement I would definitely support having target agnostic tests.
These tests are all in Java, since any end to end test involves Java for the tool.
From there the Java tests launch the target runtime as a child process.
Envoyé de mon iPhone
… Le 3 déc. 2017 à 02:09, David Morais Ferreira ***@***.***> a écrit :
@KvanTTT I am not sure about which csproj the tests should be added to, as there don't seem to be any CSharp tests. I'd greatly appreciate some pointers :)
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
I agree @ericvergnaud. I noticed that this PR didn't make it into #2146. Do I need to make further changes? |
|
Hi @DavidMoraisFerreira Just added it now so I am closing this one. |
5 participants
Ported @bramp's java implementation (#2048 and #2046) for case insensitive grammars to the CSharp runtime.