This is an archive of the discontinued LLVM Phabricator instance.

[clang-repl] Fix incorrect return code
ClosedPublic

Authored by junaire on Jul 23 2022, 6:22 AM.

Details

Summary

Without this patch, clang-repl incorrectly pass some tests when there's
error occured.

Signed-off-by: Jun Zhang <jun@junz.org>

Diff Detail

Event Timeline

junaire created this revision.Jul 23 2022, 6:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2022, 6:22 AM
junaire requested review of this revision.Jul 23 2022, 6:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2022, 6:22 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
v.g.vassilev added inline comments.Jul 23 2022, 10:51 AM
clang/test/Interpreter/fail.cpp
7

Why this is marked as XFAIL? If it is only for the return code we can add the not command in front to make the error code succeed.

v.g.vassilev accepted this revision.Jul 31 2022, 1:10 AM

Thanks for working on this. This is one of the problems where is hard to find the right behavior. For example, if a repl running in interactive issues an error and then successfully recovers what's the right process return code success or a failure? If we decide it is a success then if we ran in non-interactive mode eg clang-repl "err" then the exit code should be a failure.

Either way, this patch improves our current state in which we cannot detect failing tests in -verify mode. Let's accept it.

This revision is now accepted and ready to land.Jul 31 2022, 1:10 AM

Thanks for working on this. This is one of the problems where is hard to find the right behavior. For example, if a repl running in interactive issues an error and then successfully recovers what's the right process return code success or a failure? If we decide it is a success then if we ran in non-interactive mode eg clang-repl "err" then the exit code should be a failure.

Either way, this patch improves our current state in which we cannot detect failing tests in -verify mode. Let's accept it.

Maybe add a FIXME in the tests?

Thanks for working on this. This is one of the problems where is hard to find the right behavior. For example, if a repl running in interactive issues an error and then successfully recovers what's the right process return code success or a failure? If we decide it is a success then if we ran in non-interactive mode eg clang-repl "err" then the exit code should be a failure.

Either way, this patch improves our current state in which we cannot detect failing tests in -verify mode. Let's accept it.

Maybe add a FIXME in the tests?

That's probably a good idea.

junaire updated this revision to Diff 448844.Jul 31 2022, 2:07 AM

Rebase and add a FIXME

junaire updated this revision to Diff 448846.Jul 31 2022, 2:11 AM

Fix some typos

@v.g.vassilev if you can take another look at the wording of the FIXME, and make sure you're happy about it, that will be awesome.

v.g.vassilev accepted this revision.Jul 31 2022, 2:37 AM
v.g.vassilev added inline comments.
clang/tools/clang-repl/ClangRepl.cpp
138–140

This seems not needed. We have already llvm::llvm_shutdown_obj Y;

junaire added inline comments.Jul 31 2022, 2:47 AM
clang/tools/clang-repl/ClangRepl.cpp
138–140

This seems not needed. We have already llvm::llvm_shutdown_obj Y;

Oh good catch, I think I messed it in the rebase :(

This revision was landed with ongoing or failed builds.Jul 31 2022, 4:14 AM
This revision was automatically updated to reflect the committed changes.