Fix null references #81
Conversation
frederikja163
left a comment
frederikja163
left a comment
There was a problem hiding this comment.
I think this PR is a bit misguided as there are two seperate changes in one PR (why?) and i personally disagree with both of them. I would be happy to discuss further though. But i have listed my arguments for now.
SudoScript.Core/Parser.cs
Outdated
| { | ||
| paramChildren.Add(ParseParameterIdentifier(stream)); | ||
| throw new Exception("Parameter are expected to be separated by space"); | ||
| } |
There was a problem hiding this comment.
Not a fan of this change. Before it was an early return as a validation check. Now you have introduced an extra level of nesting across the entire function. It just reads worse and is less managable Imo.
|
|
||
| private char? _carry; | ||
|
|
||
| private readonly string _src; |
There was a problem hiding this comment.
Pretty sure this was supposed to be used to exceptions. I would probably rather that we implement those exceptions than remove the property.
|
I think it's fine to freeze this pull request for now, until the _src utility is implemented. The main reason for wanting to fix these now, was for performance tools to function properly. Since this is happening on a separate branch that likely won't be pulled anyways, I think it's fine to say that these changes need a little more work put in to them before merging. |
|
Why would you need this to be nullable for performance tools? You could make an argument it should be nullable. But i dont see why that by itself wouldnt fix it. |
|
BenchmarkDotNet won't run with possible null references in the code. I have these changes on a separate branch that won't be merged anyways. |
2 participants
Removed _src from TokenStream.cs as it was never assigned or used.
Made the out Token on line 124 of the parser nullable.