[PATCH] D98859: [lit] Handle plain negations directly in the internal shell

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added a comment.

In D98859#2652464 <https://reviews.llvm.org/D98859#2652464>, @mstorsjo wrote:

> @jdenny - Updated with some tests. The existing tests in the shtest-not suite seem to cover the uses of 'not' quite well - so just for the change of making 'not' evaluated builtin those existing tests should cover all aspects I think.

But lit's test suite doesn't check that internal `not` is actually used instead of external `not`, right?  Is that change more a side effect than a goal of this patch?

If it seems worthwhile to verify that internal `not` continues to be used, we do have `llvm/utils/lit/tests/Inputs/fake-externals`, which is a directory of fake external commands that always fail.  It's in PATH while running lit's own test suite to prove that external commands aren't called accidentally.  You could add a simple `not` script there.  The only challenge I foresee is that it would need to defer to the external `not` if `--crash` is the first argument.

> But for additionally handling '!' as equivalent to 'not' (except for the --crash option), I added a couple simple tests in the shtest-not suite - I didn't choose to duplicate all the aspects of suite but only a few trivial ones.

Thanks for adding those.

I agree you don't need to replicate all existing `not` tests for `!`, but I feel there are a few more basic cases that ought to be covered.  Most importantly, your new tests would pass even if `!` always blindly has exit status zero.  `not-calls-diff.txt` (at least) checks that `not` produces a non-zero exit status when expected.  Also, there's no test that `!` properly complains about a missing subcommand.  That's a separate code path from when `not` doesn't have a subcommand.

> To actually be able to reland this, we'd need to figure out a sensible way to handle the unrelated breakage that just surfaces when this is in, e.g. with D99330 <https://reviews.llvm.org/D99330> or something else similar.

Understood.  If possible, I'd like to leave that review to people who work regularly in windows.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98859/new/

https://reviews.llvm.org/D98859