[clang-repl] Improve error message on failed undos #149396
Conversation
used grep -r "Too many undos" to locate all instances where the string was mentioned. Changed the string to the suggestion in issue 143668
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Aaron Danen (aadanen) ChangesHi all. This is my first pull request to an open source project. I am a student so have mercy on me! I have done my best to read all of the relevant documentation about how to contribute most effectively but its totally possible I've made a mistake or missed something. Please let me know so I can do better next time :) I found a simple issue and have done my best to resolve it. I believe this change resolves #143668 All i did was grep -r "Too many undos" and replace all instances of that string with the suggested replacement. thank you for your time Full diff: https://github.com/llvm/llvm-project/pull/149396.diff 2 Files Affected:
diff --git a/clang/lib/Interpreter/Interpreter.cpp b/clang/lib/Interpreter/Interpreter.cpp
index ed3bae59a144c..a2696f78cb510 100644
--- a/clang/lib/Interpreter/Interpreter.cpp
+++ b/clang/lib/Interpreter/Interpreter.cpp
@@ -754,7 +754,7 @@ llvm::Error Interpreter::Undo(unsigned N) {
if (N > getEffectivePTUSize())
return llvm::make_error<llvm::StringError>("Operation failed. "
- "Too many undos",
+ "No input left to undo",
std::error_code());
for (unsigned I = 0; I < N; I++) {
if (IncrExecutor) {
diff --git a/clang/unittests/Interpreter/InterpreterTest.cpp b/clang/unittests/Interpreter/InterpreterTest.cpp
index b97f5ae17c9f0..2ec65e2a9b2ef 100644
--- a/clang/unittests/Interpreter/InterpreterTest.cpp
+++ b/clang/unittests/Interpreter/InterpreterTest.cpp
@@ -158,12 +158,12 @@ TEST_F(InterpreterTest, UndoCommand) {
// Fail to undo.
auto Err1 = Interp->Undo();
- EXPECT_EQ("Operation failed. Too many undos",
+ EXPECT_EQ("Operation failed. No input left to undo",
llvm::toString(std::move(Err1)));
auto Err2 = Interp->Parse("int foo = 42;");
EXPECT_TRUE(!!Err2);
auto Err3 = Interp->Undo(2);
- EXPECT_EQ("Operation failed. Too many undos",
+ EXPECT_EQ("Operation failed. No input left to undo",
llvm::toString(std::move(Err3)));
// Succeed to undo.
|
Welcome!
Simple and effective but, there is a little bit more context in this case, which I'll explain in a review comment in a moment. Some admin stuff:
Having said that, I think you'll be better able to describe the change once we've gone through some review. So leave both of those as is, for now, and we'll make sure they're ok later. Another thing that you may not have had to do for the change as is, but will when I explain a bit more, is run the tests locally. So if you haven't already, follow https://llvm.org/docs/GettingStarted.html and get to the point where you can build clang. From there you can either use In addition to that, our CI will be running tests for you but it is much more useful to have them running locally so your edit -> run -> edit loop can be faster. |
|
|
|
#149396 (comment) is one of our automated checks, please follow its guidance. The reason we do this is so we can attribute changes for reasons around the licensing of contributions, there's more info in those links. It can be any valid address, you don't have to use a "personal" one, for instance I use my email address provided by my employer (that and they require that I do, but beside the point). |
|
differentiate between the case of 0 inputs to undo and the case when there are X inputs and Y requested undos but Y > X
|
I put in inputs vs undo requests logic but I think the unit test might need to be edited as well |
|
Okay I think that does it :) |
aadanen
changed the title
DavidSpickett
changed the title
|
Looks great! CI should confirm the test is correct, and I added the clang-repl maintainer on review in case they have anything to say. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
llvm/clang is not a light build at the best of times, but there are some things you can try - https://llvm.org/docs/GettingStarted.html#common-problems If you do pick up another issue, I can give you some tailored suggestions for the specific task. For instance here, "check-clang" is runs all clang checks but I happened to know that the test you're changing is what we call a "unit test" and has it's own smaller target.
That's right. In fact I think those "merge" commits are preferred during review, versus rebasing, because GitHub likes to hide review state if you do the latter. It's also fine to not update the PR branch until review has concluded. |
|
You have formatting to fix, I suggest you just copy and paste the diff in this one time. Folks will have different ways to setup clang-format, I use the script https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting and manually run it (because I am too lazy to integrate it somewhere :) ). I also add "black" for python formatting, into one giant command: If you're using an editor, do look into applying the format on save, it's much more convenient. |
There was a problem hiding this comment.
LGTM
@aadanen you won't be able to merge this yourself but I can once you've fixed the formatting.
|
@DavidSpickett I applied clang-format and I think everything is looking good. Do I need to do anything else? Or is squashing the commits and merging up to someone with write permissions? Also, thanks a lot for the help. I plan to find a new issue to work on soon. Thanks for offering to help there as well! I bet it will save me a lot of time and confusion. |
|
@aadanen Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
No, all good.
GitHub will do the squashing, and yes I just clicked the merge button for you. Once you have established a track record you can ask for permissions to be able to do this yourself. At no point do you have to have those permissions though, so I wouldn't bother with it for a while.
If you want to stay around clang-repl, I opened #143666 around the same time. Feel free to pick anything else of interest though. |
Updated error message logic for undo function. Throws different errors for the case of there being nothing to undo, and for the case of requesting more undos than there are operations to undo. Fixes llvm#143668
4 participants
Updated error message logic for undo function. Throws different errors for the case of there being nothing to undo, and for the case of requesting more undos than there are operations to undo.
Fixes #143668