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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

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

> 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.
> I may have to agree to disagree.

Generally deprecation is used when there's a lot of use we can't practically clean up in the short term - if all the uses are removed I think it's appropriate to remove the functionality. Otherwise I think it'll be pretty easy for new uses to creep in.

& yeah, if it's only this one point of confusion - I guess it's a bit hard to say if this is an improvement. (on the one hand if this is all the usage, it's not a lot of usage so people haven't come across this much - but on the other hand folks don't seem to have had trouble with it so far)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83578