[PATCH] D150856: [lit] Add %{for-each-file} substitution

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1654
+            next(match_iter)
+            raise ValueError(
+                "Multiple %{for-each-file} per directive are not supported"
----------------
Surely there must be a simpler way to do this. One could turn the match iterator into a list (`list(match_iter)`) and raise an error if the length is more than 1.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1663
+            raise ValueError(
+                "%{for-each-file} expects an absulute path,"
+                " but '{}' is not absolute."
----------------
typo absolute


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1699
         processed = processLine(ln)
         while processed != ln and steps < recursion_limit:
             ln = processed
----------------
Depending on whether we've done a for each file substitution, this `!=` will now do very different things. I recommend making the return type of processLine uniform one way or another, probably to always return a list of strings.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1724
                 line = directive
-            output.append(unescapePercents(process(line)))
+            line = unescapePercents(process(line))
+            if isinstance(line, str):
----------------
I recommend returning a list for simplicity, and always extending.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150856