[clang-repl] add %help, documentation, and tests for %commands

[aadanen]

1. Added %commands to documentation
2. Added %help command to clang repl
3. Expanded parsing to throw unique errors in the case of users entering an invalid %command or using %lib without an argument
4. Added tests to check the behvaior of %help, %lib, and bad %commands

[llvmbot]

@llvm/pr-subscribers-clang

Author: Aaron Danen (aadanen)

Changes

so far just added undo to the documentation. We can see about implementing %help or something similar. This will attempt to solve issue #143666

---

Full diff: https://github.com/llvm/llvm-project/pull/150348.diff

1 Files Affected:

• (modified) clang/docs/ClangRepl.rst (+7)

diff --git a/clang/docs/ClangRepl.rst b/clang/docs/ClangRepl.rst
index 5399036c123fb..9ef82df28ee43 100644
--- a/clang/docs/ClangRepl.rst
+++ b/clang/docs/ClangRepl.rst
@@ -197,6 +197,13 @@ Comments:
    clang-repl> // Comments in Clang-Repl
    clang-repl> /* Comments in Clang-Repl */
 
+Undo:
+=====
+
+.. code-block:: text
+
+   clang-repl>%undo
+
 
 Closure or Termination:
 =======================

[aadanen]

I thought about "Undoing Input" as the section header but I feel like %undo is pretty self explanatory. As far as "%help" and clang-repl --help go, I think those sound like nice ideas. Both would be useful for new people using clang-repl.

As far as "%foobar" throwing an error like "invalid command, enter %help for list of commands" I think it would be okay. The only use of '%' I know of in cpp is the modulo operator which would never be the first character in a line

[aadanen]

@DavidSpickett I've added %undo to the documentation and implemented %help. Would it be good to also add %help to the documentation?

To print stuff, I am just using printf(). Is this fine? It would be easy to switch to std::cout or it looks like there are some llvm print functions but I am not sure if its necessary/preferred to use them.

As far as %something throwing a specific error about a bad %command, I feel like there are too few commands and they are too easy to type for it to be needed. Let me know if you think its needed and I will implement it.

Thanks
aadanen

[DavidSpickett]

I've added %undo to the documentation and implemented %help. Would it be good to also add %help to the documentation?

Yes add %help. It feels a bit redundant and it is, but it'll also be a hint for future developers to say "hey, remember that new commands have to appear in %help too".

To print stuff, I am just using printf(). Is this fine? It would be easy to switch to std::cout or it looks like there are some llvm print functions but I am not sure if its necessary/preferred to use them.

Generally we'd use some llvm:: thing mostly because it'll be writing to a stream that can be redirected for testing and other purposes. The best idea is to look at how the surrounding code operates and follow that style.

Also when you add tests you'll see that capturing stdout is more annoying than passing a string backed stream to whatever you're testing, and reading back from the string.

Overall though, using the "obvious" functions just to get a thing working and then swapping to the project specific APIs is a good approach. Even if you stuck with printf, all that would happen is a reviewer would say hey this output is fine but you need to use this API instead, which is an easy fix.

The only use of '%' I know of in cpp is the modulo operator which would never be the first character in a line

I don't keep up with the standards but I seem to remember a change that relaxed some restricted characters Probably for C++26 at the earliest though. You are right that % in normal circumstances is just modulo.

Ofc maybe someone does a %b and would prefer to see the compiler error, so if the line (stripped of surrounding whitespace) is not just %thing, then it does not make sense to treat it as a % command.

As far as %something throwing a specific error about a bad %command, I feel like there are too few commands and they are too easy to type for it to be needed. Let me know if you think its needed and I will implement it.

My point was how do you even discover that the % thing exists and how do you see typos? Yes you can compare to the docs but:

> %unod
Invalid % command: unod

Puts it all closer together so I don't have to look at docs. If it did the "did you mean undo?" thing that'd be even better but leave that for another time.

I agree this part is not necessary just nice to have, so I would leave it out and if you have time later come back to it.

It's just one of those "nice" things I have got used to in many other (more complex) programs. Where program --option=something_random_I_made_up responds with Invalid --option value, you could have used these instead:.

[DavidSpickett]

To be clear, not saying you have to do all of the things suggested here. If you find something else to work on, please do :)

[aadanen]

added %help to docs
used llvm::raw_ostream instead of printf() but didn't go all the way to making it a whole testable abstracted argument thing for easier testing. If this is important I can edit it. Also let me know if I should use a different utility class than raw_ostream
added handling for bad % commands

this solves #143666

I have implemented a very simple solution to this problem. Hardcoded strings are good because they are easy to understand and easy to change. However, it might be "better architecture" to make some sort of percentCommand object/struct/class thingy. each instance could have a name, description, and maybe even like a function pointer or something to what should happen if we encounter that command.

I don't think its necessary yet because theres so few % commands and it doesn't look like there are plans to add more at the moment. If we want more architecture around % commands that can be another issue/feature i think.

Let me know if I should add anything else :)

[aadanen]

Looks like there are some issues. I only ran check-clang-unit before pushing so I'll have to sort those out

[aadanen]

1. printing stuff on clang-repl start causes several tests to fail
2. need to not parse %lib as a bad % command. Maybe should add %lib as list of % commands?

[DavidSpickett]

Sorry, I didn't realise this was a PR! So tunnel visioned on the comment notification. So if any of what I said made not much sense, that's why :)

Will review this today.

[DavidSpickett]

I had a dark thought that maybe mutli-line input would mess up checking for % commands, but luckily it does not.

clang-repl> int a = 1;
clang-repl> int b = 10 \
clang-repl...   %undo
In file included from <<< inputs >>>:1:
input_line_2:1:13: error: use of undeclared identifier 'undo'
    1 | int b = 10 %undo
      |             ^~~~
input_line_2:1:17: error: expected ';' after top level declarator
    1 | int b = 10 %undo
      |                 ^
      |                 ;
error: Parsing failed.

If it was doing as I feared, it would have treated that as a command, but it does not. I think it's seeing the two input lines as one, with an internal newline.

% commands have to start with % after leading and trailing whitespace has been removed. This is also why %undo works.

clang-repl>          %undo
error: Operation failed. No input left to undo

[DavidSpickett]

You mention that printing this causes a bunch of tests to fail, which is to be expected but probably a sign we should leave this feature out of this PR. It may be something the maintainer would rather not have, and fixing all the tests in this PR will add a lot of things for us to review.

What you've done is good though, that's what I was thinking of.

I didn't mention it before but gdb does this:

$ gdb
GNU gdb (Ubuntu 12.1-0ubuntu1~22.04.2) 12.1
<...>
For help, type "help".
Type "apropos word" to search for commands related to "word".
(gdb) q
$ gdb --quiet
(gdb) q

Also clang-repl may want to detect if it's outputting to an interactive terminal or not. Which I did not think of before.

(GDB does not, it expects you to use --quiet)

So yeah, leave this bit out so we don't distract from the rest, maybe come back to it in future.

[aadanen]

I believe I implemented everything you suggested. Thanks for the in depth review!

[DavidSpickett]

So this is great, just some small things to fix.

Next you need to add tests for:

• %help
• %lib with no argument
• %rubbish error message

I would do all those as their own test files. You could add the %lib bit to one of the existing tests, but each file seems to have a specific thing it's checking, so new files seems ok.

The maintainer can disagree later if not.

[aadanen]

It is a little awkward for %help to use logAllUnhandledErrors because the message goes to stderr and then my test needs to do input redirection. There is likely a better way to do this but I haven't found it yet, unless it's the raw ostream thing from earlier

[DavidSpickett]

Use the ostream then. We'll see if other reviewers think differently.

[DavidSpickett]

Please update the PR description to reflect the current changes.

@vgvassilev what do you think of this?

Only question I have is whether there's a special way to print non-error output, or whether stdout is fine? In theory %help is just like `printf("..."); but it prints some set text.

[aadanen]

I've made the requested changes, and figured out how to edit a PR description. I also tried to write a more concise title.

[DavidSpickett]

Looks good, thanks!

Let's see what the maintainer has to say.

[vgvassilev]

Please update the PR description to reflect the current changes.

@vgvassilev what do you think of this?

Quite nice piece of work. Than you both for doing this!

Only question I have is whether there's a special way to print non-error output, or whether stdout is fine? In theory %help is just like `printf("..."); but it prints some set text.

I am not aware of any other way to achieve that and maybe stdout is just fine.

[vgvassilev]

LGTM! Probably more changes to the documentation is required but that's a good step! Thank you!

[aadanen]

Nice!