[PATCH] D78589: [lit] Add an option to print all features used in tests

[Julian Lettner via Phabricator via llvm-commits]

yln added a comment.

Thanks for being thorough and explaining your reasoning so well.  I now agree that "Unavailable Features" introduces too much noise to simply add it to the existing option without the user asking for it.

Two more quick questions:
We present features as a sum of all used features, however they are specific to configs/test suites. Could we group them by test suite? Maybe a good way to do this would be to extend the print_discovered function. This way we can also combine output from the  "show and exit" options.

Currently we have "Available Features" in `--show-suites`, which can be either used or unused.  When the user also asks for `--show-used-features` then maybe it would still be helpful to somehow mark the unused ones without introducing too much visual noise (e.g., with an asterisk + footnote).  This would help with "de-crufting" in at least one direction.



================
Comment at: llvm/utils/lit/lit/Test.py:376
+            BooleanExpression.tokenize(expr) for expr in
+                itertools.chain(unsupported, requires, xfails) if expr != '*'
+        )
----------------
ldionne wrote:
> yln wrote:
> > Prefer being dry over performance, remove `if expr != '*'`
> It doesn't work when `expr == *`, that's why I added it :-). The problem is that `'*'` is not a valid value for a `BooleanExpression`, however `'*'` is still used in `XFAIL:` because it has a special meaning there (see https://github.com/llvm/llvm-project/blob/master/llvm/utils/lit/tests/Inputs/shtest-format/requires-star.txt and https://github.com/llvm/llvm-project/blob/master/llvm/utils/lit/lit/Test.py#L285).
Got it!  Thanks for explaining.


================
Comment at: llvm/utils/lit/lit/cl_arguments.py:160
+    debug_group.add_argument("--show-features",
+            help="Show all features used in the test suite (in XFAIL, UNSUPPORTED and REQUIRES) and exit",
+            action="store_true")
----------------
ldionne wrote:
> yln wrote:
> > Show features used by selected tests; or use discovered_tests below
> Done. However, I do think it would make more sense for all of them to use the `filtered_tests` instead, don't you think? The argument is that it's simply more general: no filter is equivalent to `discovered_tests`, but it also allows filtering if one wants.
> 
> Should I create a follow-up patch?
That makes sense.  Feel free to send a patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78589