JBenda / inkcpp

Inkle Ink C++ Runtime with JSON>Binary Compiler
MIT License
70 stars 13 forks source link

New value implementation #27

Closed JBenda closed 3 years ago

JBenda commented 3 years ago

New implementation for values at the evaluation stack, and operation handling for evaluation stack operations.

brwarner commented 3 years ago

Review is still in progress. I've read through everything once but I want to go over it again more thoroughly now that I understand a bit better what's going on. I like that it simplifies a lot of the ad-hoc decisions I had to make around the value system as I extended it to meet all its needs. A lot of the constexpr code around the casting matrix is really confusing but I'm slowly figuring it out. I'm not sure how I feel about how many lines have been added in template specializations but I suppose they are all optimized out by the compiler. It becomes quite hard to follow but admittedly GitHub is serving me the files in a non-intuitive order. For a lot of those files it might be nice to put a header comment at the top just explaining the purpose of the file so a reader doesn't have to puzzle through all the templates as much. I did read the new OperationNotes.md file which helped.

I can see however that this will make adding lists a lot easier since they add a whole host of new value operations that the rest of the types do not need to handle.

JBenda commented 3 years ago

I added some descriptions. Thanks for working through all of this. Exactly the template specializations are more lines but there for the compiler don't need to look up at runtime, and we don't need to check for each value_type which return value its provide (which also will change)

JBenda commented 3 years ago

Oh, now that's test also failed, interesting, probably related to the error in #26

JBenda commented 3 years ago

Included in #17