[PATCH] D93078: [utils] Fix UpdateTestChecks case where 2 runs differ for last label

[Mircea Trofin via Phabricator via cfe-commits]

mtrofin added a comment.

In D93078#2500124 <https://reviews.llvm.org/D93078#2500124>, @jdoerfert wrote:

> In D93078#2500122 <https://reviews.llvm.org/D93078#2500122>, @mtrofin wrote:
>
>> In D93078#2500114 <https://reviews.llvm.org/D93078#2500114>, @jdoerfert wrote:
>>
>>> In D93078#2500040 <https://reviews.llvm.org/D93078#2500040>, @mtrofin wrote:
>>>
>>>> In D93078#2500032 <https://reviews.llvm.org/D93078#2500032>, @jdoerfert wrote:
>>>>
>>>>> In D93078#2499996 <https://reviews.llvm.org/D93078#2499996>, @mtrofin wrote:
>>>>>
>>>>>> In D93078#2499995 <https://reviews.llvm.org/D93078#2499995>, @jdoerfert wrote:
>>>>>>
>>>>>>> I'm not sure how this is more helpful. What is the use case where this way of warning helps?
>>>>>>
>>>>>> For tests other than attributor, that explicitly set FileCheck --allow-unused-prefixes=true, these warnings mean that there will be unused prefixes (those listed)
>>>>>
>>>>> Should not we check for that flag in the RUN line then and only warn for unused prefixes when it is set. If there is no prefix we should obviously always warn.
>>>>
>>>> That's a good idea. Probably we'd need to also make sure that the unused prefixes are all on RUN lines with --allow-unused-prefixes=true.
>>>>
>>>> I'm also not sure how lit.local.cfg interacts with the test prefix updater: currently, the only cases where we bulk-want to allow unused prefixes is the Attributor tests. If you were going to add the flag explicitly, that'd also work. Or just the option to the update_test_prefix that says "ok with duplicates, don't warn"
>>>
>>> I can add the option explicitly (D94744 <https://reviews.llvm.org/D94744>). We should look for the filecheck one if possible, two options means double the hassle. That said, why are we warning in both FileCheck and update_test_check, it seems to be unnecessary to do the latter.
>>
>> update_test_prefix.py's role is temporary: once we flip FileCheck to disallow unused prefixes by default, we don't need to keep it around. At that point, it becomes important for the update_<xyz>_test_check scripts to warn.
>
> I don't follow. I would assume it's the opposite. If FileCheck doesn't allow unused prefixes why warn in the update script as well. Anyway, there should be a way to opt-out.



In D93078#2500124 <https://reviews.llvm.org/D93078#2500124>, @jdoerfert wrote:

> In D93078#2500122 <https://reviews.llvm.org/D93078#2500122>, @mtrofin wrote:
>
>> In D93078#2500114 <https://reviews.llvm.org/D93078#2500114>, @jdoerfert wrote:
>>
>>> In D93078#2500040 <https://reviews.llvm.org/D93078#2500040>, @mtrofin wrote:
>>>
>>>> In D93078#2500032 <https://reviews.llvm.org/D93078#2500032>, @jdoerfert wrote:
>>>>
>>>>> In D93078#2499996 <https://reviews.llvm.org/D93078#2499996>, @mtrofin wrote:
>>>>>
>>>>>> In D93078#2499995 <https://reviews.llvm.org/D93078#2499995>, @jdoerfert wrote:
>>>>>>
>>>>>>> I'm not sure how this is more helpful. What is the use case where this way of warning helps?
>>>>>>
>>>>>> For tests other than attributor, that explicitly set FileCheck --allow-unused-prefixes=true, these warnings mean that there will be unused prefixes (those listed)
>>>>>
>>>>> Should not we check for that flag in the RUN line then and only warn for unused prefixes when it is set. If there is no prefix we should obviously always warn.
>>>>
>>>> That's a good idea. Probably we'd need to also make sure that the unused prefixes are all on RUN lines with --allow-unused-prefixes=true.
>>>>
>>>> I'm also not sure how lit.local.cfg interacts with the test prefix updater: currently, the only cases where we bulk-want to allow unused prefixes is the Attributor tests. If you were going to add the flag explicitly, that'd also work. Or just the option to the update_test_prefix that says "ok with duplicates, don't warn"
>>>
>>> I can add the option explicitly (D94744 <https://reviews.llvm.org/D94744>). We should look for the filecheck one if possible, two options means double the hassle. That said, why are we warning in both FileCheck and update_test_check, it seems to be unnecessary to do the latter.
>>
>> update_test_prefix.py's role is temporary: once we flip FileCheck to disallow unused prefixes by default, we don't need to keep it around. At that point, it becomes important for the update_<xyz>_test_check scripts to warn.
>
> I don't follow. I would assume it's the opposite. If FileCheck doesn't allow unused prefixes why warn in the update script as well. Anyway, there should be a way to opt-out.

There's an earlier comment in this patch about that, anchored to line 296 of common.py. IIUC, a developer: 1) updates their test by adding new RUN lines, maybe adding prefixes to existing RUN lines, 2) runs the appropriate update_xyz_checks.py. Then, indeed, they could run the test and see FileCheck's warnings, but they might be confused. Instead, we can pre-warn. Eventually, we could imagine giving a bit more information in the warning as to which RUN line that was, for example.

I'll look at adding --allow-unused-prefixes=true awareness shortly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93078