================
@@ -370,6 +379,10 @@ int main(int argc, const char **argv) {
if (Input == R"(%undo)") {
if (auto Err = Interp->Undo())
llvm::logAllUnhandledErrors(std::move(Err), llvm::errs(), "error: ");
+ } else if (Input == R"(%help)") {
+ OS << help_output << '\n';
+ } else if (Input[0] == '%') { // make sure this is evaluated last
----------------
DavidSpickett wrote:
Ok there's some nuance here. `%lib` is being handled specially and my instinct
was that you do this invalid command check after that, but that creates a
problem because `%lib` with no argument then returns an error saying it's an
invalid command.
I see that the line is trimmed earlier. So if we type in `%lib ` then it
will become `%lib`. What you could do is look for `%lib` on its own and error
something like: `Command "%lib" requires 1 argument, the path to a library
file.".
After that comes the second `%lib ` check. That knows that it must be seeing
`%lib <some argument>`, or something completely different.
Then comes your invalid command check which catches anything else that starts
with `%`.
Does that make sense?
* Is the input literally `%help`?
* Is the input literally `%undo`?
* check all the other literal commands that take no arguments....
* Is the input literally `%lib`? If so, error because we want 1 argument for
that
* Is the first char of the input `%`? If so error unknown command
* Else process as C/C++ input
I'm looking at the tests and there are 2 `%lib` tests and neither check the
error messages. This should be improved, if they did then you would have seen
this problem earlier.
(and to be clear, don't feel bad about that, this is what happens when you have
bespoke hand crafted if/else parsing, it comes with the territory)
So if this were me, I would open a new PR and add extra tests for the `%lib`
command. Without changing anything about the implementation. Then when that's
landed, rebase this PR to include that. Now I've got great coverage that'll
tell me if I got anything wrong.
This is often referred to as "pre-comitting tests". It means that you document
the current behaviour in the form of tests, then change that behaviour. And the
change will have a much smaller diff than if you did it all in one.
https://github.com/llvm/llvm-project/pull/150348
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
