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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
