Small changes for string parsing

[sungam3r]

This PR represents a result for the review of #131 . Great job @Shane32 . I did not thoroughly check the entire written code. I confess that it is difficult to read. Many counters, many branches. Nevertheless, the general meaning is well understood. Excellent that you wrote all those tests. In addition to cosmetic edits and comments, I reduced the size of the buffers used on the stack. I do not think that we needed such large buffers.

[sungam3r]

@Shane32 I am 100% sure that this can be done, but I was not so brave to do it myself :).

[Shane32]

Great job @Shane32 .

Thanks! The spec is fairly tricky to implement correctly in an optimized manner. But I opted for optimization rather than using LINQ or something more readable. I tried to put some comments in the code to help make it understandable.

Looking back at it now, I don't like that it uses try..catch to catch an overflow and then handle it. As a general rule of thumb I try not to let any code generate exceptions when no error has occurred for two reasons: it significantly slows down execution of code that does not generate an error (in this case, any blockstrings over 4000 chars), and (2) while debugging user code with 'just my code' turned off, the debugger may break on the exception when no error has truly occurred.

If you agree, perhaps you want to modify the PR... up to you.

[codecov-commenter]

Codecov Report

Merging #140 (811f2c0) into master (69342d5) will increase coverage by 0.59%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##           master     #140      +/-   ##
==========================================
+ Coverage   92.06%   92.66%   +0.59%     
==========================================
  Files          48       49       +1     
  Lines        2193     2317     +124     
  Branches      251      273      +22     
==========================================
+ Hits         2019     2147     +128     
+ Misses        147      143       -4     
  Partials       27       27              

Impacted Files	Coverage Δ
src/GraphQLParser/LexerContext.cs	92.38% <87.50%> (+0.34%)	⬆️
src/GraphQLParser/ParserContext.Parse.cs	97.96% <0.00%> (-0.06%)	⬇️
src/GraphQLParser/AST/GraphQLTypeDefinition.cs	100.00% <0.00%> (ø)
src/GraphQLParser/AST/GraphQLDescription.cs	50.00% <0.00%> (ø)
...rc/GraphQLParser/AST/GraphQLDirectiveDefinition.cs	75.00% <0.00%> (+12.50%)	⬆️
...c/GraphQLParser/AST/GraphQLObjectTypeDefinition.cs	87.50% <0.00%> (+12.50%)	⬆️
...rc/GraphQLParser/AST/GraphQLUnionTypeDefinition.cs	71.42% <0.00%> (+14.28%)	⬆️
...raphQLParser/AST/GraphQLInterfaceTypeDefinition.cs	85.71% <0.00%> (+14.28%)	⬆️
...c/GraphQLParser/AST/GraphQLScalarTypeDefinition.cs	66.66% <0.00%> (+16.66%)	⬆️
src/GraphQLParser/AST/ASTNode.cs	100.00% <0.00%> (+50.00%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69342d5...811f2c0. Read the comment docs.

[sungam3r]

Looking back at it now, I don't like that it uses try..catch to catch an overflow and then handle it

Your ReadBlockString does not use try/catch, ReadComment/ReadString do.

[Shane32]

Looking back at it now, I don't like that it uses try..catch to catch an overflow and then handle it

Your ReadBlockString does not use try/catch, ReadComment/ReadString do.

Oh. Didn't think I would have done that. Well, sometime we can make an improvement there.

[sungam3r]

In fact, I would not worry because of a potential exception handling slowdown. If an exception is happening, then not so often. In addition, the code performed under the try/catch block can work faster so without this block. I once read about such compiler issue.

[sungam3r]

Waiting a bit for graphql/graphql-spec#899 to verify naming.

[Shane32]

It just really annoys me when I go to debug something, and VS breaks on some internal line of code, which isn't an error. Which just reminds me that there's a significant performance problem whenever that bit of code gets hit.

[sungam3r]

I know and understand.

[sungam3r]

@Shane32 see graphql/graphql-spec#899 (comment) . Nevertheless, I will keep string and block string naming in error messages since there is no no-block-string term, only BlockString and StringValue that includes BlockString.