[PATCH] D83578: [test] Replace a fragile lit feature (substitution in an argument place) with command -v

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D83578#2710454 <https://reviews.llvm.org/D83578#2710454>, @MaskRay wrote:

> The utilities and the shell are all of Unix style. It is well established that argv[0] is magic and the shell/OS can find it from `PATH`. `argv[i] (i!=0)` is not and an argument such as`ls`, `ln`, `llvm`, and `objdump` in a non argv[0] place does not do anything special.
> For example, `echo ls` prints `ls`, not the absolute path to `ls`. I think Windows cmd.exe behaves this way as well. Some three-letter utilities are already widely used, and the replacement at non-argv[0] place can be quite confusing `echo llc` => /path/to/llc.

I get what you're saying, but that just isn't how I think about the test, if I'm honest. When I read it, I see a reference to an executable, and assume that this is the executable under test, rather than just an arbitrary string that happened to match a tool name. I could see changing the behaviour to not do the substitution to have the potential to cause confusion.

> There is a little mental gap for readers who haven't used `command -v` before, but this probably still looks better than `ln -s llvm-ranlib ....` which gives a false impression that this command is incorrect.

It doesn't give a false impression to me, but maybe I'm an exception rather than the rule?

> We could also add a replacement and use `ln -s %llvm-objdump ...` but all uses except the few tool-name.test tests use the raw names. The replacement rules seem a bit overkill (needs some code in `lit.*.py` files.)

Agreed.

I don't view the problem in the same way as you do (in fact, I don't view it as a problem at all), and as said before, I find the new test harder to follow than the old test. That being said, if multiple others view it differently to me, don't let me stop this proceeding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83578