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

Mon Apr 5 09:34:03 PDT 2021

jdenny accepted this revision.
jdenny added a comment.
This revision is now accepted and ready to land.

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

> Improved the testcases for '!'.

Thanks.  LGTM except of course the windows problem you're addressing elsewhere.

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

> In D98859#2662417 <https://reviews.llvm.org/D98859#2662417>, @mstorsjo wrote:
>
>> Adding a fake external for the 'not' executable is a bit tricky though... With the code as is, for a sequence of 'not not --crash <cmd>', it deduces that there's a '--crash' option in there, and chooses not to switch to the internal not for the outer invocation either.
>>
>> To fix that case, I'd have to change the code to peel off as many 'not' invocations from the top until the first one with a '--crash' option. Not sure if that's worth the extra complexity for the implementation - what do you think?
>
> @jdenny - with this change to the functionality, I could add a fake-external `not` to to the tests, to make sure we always evaluate all the leading instances of `not`/`!` internally, up to the first `not --crash`:

With the change you proposed, what happens to `not --crash not cmd`?  Won't the inner `not` be caught as a fake external?  I suppose the `not --crash` would fail either way, but stderr would be different, and it would be more confusing to debug.

It seems to me this is getting too confusing.  Sorry, my fake external suggestion is probably not worth the effort right now.  Thanks for looking into it.

If you agree with that assessment, would you extend the `fake-externals` comments in `llvm/utils/lit/tests/lit.cfg` to mention why `not` isn't included?  You might say: Don't do this for 'not' because lit uses the external 'not' throughout a RUN line that calls 'not --crash'.

Eventually, it would be great to see all this handling simplified.  It seems one way to do that is to implement `not --crash` in python.  But that's a different patch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98859