Conversation
| if (Regex.IsMatch(macrosValue, $"^(0|[1-9][0-9]*)$")) | ||
| return int.Parse(macrosValue); | ||
|
|
||
| if (Regex.IsMatch(macrosValue, $"^(-[1-9][0-9]*)$")) |
There was a problem hiding this comment.
Since the change should be in CPreprocessorExpresisonParser I think that's change not applicable
There was a problem hiding this comment.
does this mean that this function should not return a negative number?
There was a problem hiding this comment.
No. We belive should not have negative numbers as tokens, expression can have them just fine. Negaive numbers can be treated as unary expression - Token, for example. So likely you should expand expression grammar in the CPreprocessorExpressionParser
There was a problem hiding this comment.
if unary expression with minus exists, I left it
There was a problem hiding this comment.
I strongly suspect that we have other issue here. Numbers are tokens without sign. Sign + or - handled by Unary expression, but that does not work for some reasons.
There was a problem hiding this comment.
So probably something wrong with out Regex for PreprocessorToken
Cesium/Cesium.Preprocessor/CPreprocessorTokenType.cs
Lines 32 to 33 in e0091ae
Also I found issue in Yoakke, which generates lexer for us. LanguageDev/Yoakke#186 maybe that's related.
There was a problem hiding this comment.
I strongly suspect that we have other issue here. Numbers are tokens without sign. Sign + or - handled by Unary expression, but that does not work for some reasons.
currently, PreprocessorToken recognize number with operator - (sub) here.
Now Numbers are tokens without sign but with operator sub and etc.
There was a problem hiding this comment.
No it's not, otherwise how do you get token -10 so you need to handle it here.
There was a problem hiding this comment.
i think error here
double \ is incorrect.
There was a problem hiding this comment.
currently, the unary expression looks like this
the Result from this function will be -10. is it not correct?
I don't understand how should be
| || Regex.IsMatch(searchValue, "^0[0-7]+$") | ||
| || Regex.IsMatch(searchValue, "^0b[01]+$")) | ||
| || Regex.IsMatch(searchValue, "^0b[01]+$") | ||
| || Regex.IsMatch(searchValue, "^(-[1-9][0-9]*)$")) |
There was a problem hiding this comment.
Since the change should be in CPreprocessorExpresisonParser I think that's change not applicable
| int Parse(Location location, string macrosValue) | ||
| { | ||
| if (Regex.IsMatch(macrosValue, $"^(0|[1-9][0-9]*)$")) | ||
| if (Regex.IsMatch(macrosValue, $"^(0|-*[1-9][0-9]*)$")) |
There was a problem hiding this comment.
Also we probably should support -0
There was a problem hiding this comment.
Yeap, -0 is supported by c.
fix
There was a problem hiding this comment.
too.
+-...+-+0 and -+...-+-0 are supported. in these cases the result will be 0.
In other words, the result will always be without a sign.
+-+4 => 4
There was a problem hiding this comment.
sorry,
If count of - is even then number otherwise -number
| int Parse(Location location, string macrosValue) | ||
| { | ||
| if (Regex.IsMatch(macrosValue, $"^(0|[1-9][0-9]*)$")) | ||
| if (Regex.IsMatch(macrosValue, $"^(-?|\\+?)(0|[1-9][0-9]*)$")) |
There was a problem hiding this comment.
(Minor) I believe we can simplify this to (-|\\+)?
In plain English, the current code says "either an optional minus sign or an optional plus sign", while the new one would say "a minus or plus sign, optional", which I think better describes the intention.
|
@kant2002 are you okay with the resulting changes? I intend to merge them, unless there are some unresolved discussions left. |
|
@nt-devilboi Thank you! Was distracted and it slip out of my radar. Don't hesitate and ping me next time |
|
One of my functions here is to ping you if I see a stale discussion :) |
Okay |
3 participants
No description provided.