erichkeane added a comment.

So you're right, we haven't seen much work on this interpreter in a while, and 
I have no real experience with reviewing it at all, which I'm sure is the 
experience of most of my co-reviewers.

At the same time, I believe there is good potential in this interpreter, so I 
would love to see progress on it.

SO, if you could start by splitting this patch up into the smallest parts 
possible, it would be really appreciated.  This patch seems to be doing a lot, 
and I'm a bit overwhelmed trying to review it all at once.

Additionally, I suspect there is going to be a LOT of tests, so I'd suggest 
writing 1 whole 'test file' per topic (instead of throwing everything into 
interp.cpp).  I also suspect that there is need for more in depth testing than 
that.  For exmaple, 'if' statements likely need to check things like 
side-effects and init statements/etc.

One valuable strategy might be to temporarily switch this over to the 'default' 
interpreter in your local workspace, make changes, and see if you can add a new 
'run' line to any existing tests that each individual change you make causes to 
pass.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131888/new/

https://reviews.llvm.org/D131888

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to