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

Fangrui Song via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Apr 25 15:53:18 PDT 2021


MaskRay added a comment.

In D83578#2713370 <https://reviews.llvm.org/D83578#2713370>, @dblaikie wrote:

> In D83578#2713345 <https://reviews.llvm.org/D83578#2713345>, @MaskRay wrote:
>
>> In D83578#2713279 <https://reviews.llvm.org/D83578#2713279>, @dblaikie wrote:
>>
>>> FWIW I'm pretty ambivalent either way on this.
>>>
>>> But this patch says it replaces the feature - which I don't think it does, since it doesn't remove the functionality from lit (& not sure if replaces all uses of the feature). I think if it's going to be removed, the functionality should be removed or we'll get more of it & more confusion about which way things should be done.
>>
>> It replaces a usage, not the feature. The usage would be the blocker if we remove the functionality from lit.
>
> Right - what I mean is I don't think it's super valuable to remove the usage without also removing the functionality (& part of that - knowing we've removed all usage of the feature)

This likely removes all the usage. I hoped someone more familiar with lit can remove the functionality.

I think avoiding using a deprecated feature justifies a patch, not necessarily removing the functionality in the same patch.
However, from the patch I see some people may disagree that this is a deprecated feature.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83578



More information about the llvm-commits mailing list