https://github.com/ilovepi requested changes to this pull request.

This is pretty far from what I 'd expect at this point. There's significant 
structural problems. several of the APIs don't look remotely correct, and the 
testing is non-existent. I'm also concerned by how much allocation I see, where 
I'm pretty sure there should be zero.

Every parser API needs tests. Complete tests that show how they behave with all 
kinds of edge case tests. This includes APIs you've created for classification.

Next, I'd highly recommend you use actual types to encapsulate this logic and 
to help ensure that the construction is done correctly. Further, I'd suggest 
you model this parser as a state machine, so that you cannot transition between 
states incorrectly. I'd also suggest you think more seriously about how the 
parser needs to evolve, both for more Block types, but also based on your 
proposal for the inner types. I believe you're going for a two pass parser, but 
right now that is stated nowhere and is completely unclear.

https://github.com/llvm/llvm-project/pull/208003
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to