tbaeder added a comment. Thanks for the fast reply, Erich!
In D131888#3723017 <https://reviews.llvm.org/D131888#3723017>, @erichkeane wrote: > 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. Sure, that's expected. > 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. I actually have several smaller patches locally but merged them into one for review :) I'll try to find a middle way. > 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. I tried running the clang testsuite locally with the new interpreter, but basically all tests fail because something is missing, so I think that's gonna have to wait. But adding more files and tests on a per-patch basis is a good approach. 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
