[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#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:
>>
>>> In D93078#2499982 <https://reviews.llvm.org/D93078#2499982>, @mtrofin wrote:
>>>
>>>> In D93078#2499666 <https://reviews.llvm.org/D93078#2499666>, @jdoerfert wrote:
>>>>
>>>>> I think this caused a lot of problems for the Attributor tests. I get the "conflict output" warning all the time now :(
>>>>>
>>>>> These two `UpdateTestChecks` tests also generate the warning. I would think that they haven't before.
>>>>>
>>>>>   LLVM :: tools/UpdateTestChecks/update_test_checks/check_attrs.test
>>>>>   LLVM :: tools/UpdateTestChecks/update_test_checks/prefix-never-matches.test
>>>>>
>>>>> I suspect the `--check-attributes` flag effect are not taken into account somewhere but I might be wrong.
>>>>
>>>> The warnings are correct, but their purpose is to inform. For example, take check_attrs. The third and fourth opt run produce different function attributes from those produced by the first two, so the function is different insofar as update_test_prefix is concerned. The warnings are there to indicate that the listed prefixes end up not being used. But, that's 'by design' for the attributor tests, IIRC.
>>>>
>>>> If the warnings are noise for you, we could add a flag to quiet them down, which you'd flip when regenerating attributor tests?
>>>
>>> TBH, before, the warnings meant there is a problem that needs fixing. Now they mean, there might be one or not. So, depending on your setup it's just noise while it was pure signal before.
>>> 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"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93078