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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

In D98859#2653046 <https://reviews.llvm.org/D98859#2653046>, @jdenny wrote:

> 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?

The main target is to make '!' work for negations when using the internal shell, as there's no '!' executable. (Libcxx uses '!' as it can be tested without building llvm executables.) Technically it would be enough to only make '!' a builtin and keep 'not' external - but that might be a bigger mess implementation wise. (That would sidestep the whole windows arg quoting can of worms that I opened though.)

> 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.

Ah, good point, I can try to add that.

> The only challenge I foresee is that it would need to defer to the external `not` if `--crash` is the first argument.

Hmm, maybe if one updates $PATH to remove the path containing `$0` and then executes `not` again...

>> 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.

Thanks, those are good points. Will add more tests for those cases.

>> 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.

Yeah, definitely :-) It's quite a mess...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98859