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

[Vlad Serebrennikov via Phabricator via llvm-commits]

Endill added a comment.

Thank you for the review!



================
Comment at: llvm/utils/lit/lit/TestRunner.py:1654
+            next(match_iter)
+            raise ValueError(
+                "Multiple %{for-each-file} per directive are not supported"
----------------
rnk wrote:
> 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.
Done!


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


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1699
         processed = processLine(ln)
         while processed != ln and steps < recursion_limit:
             ln = processed
----------------
rnk wrote:
> 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.
I haven't found an easy way to make this algorithm work with `processLine()` that could generate additional directives. Given that `%{for-each-file}` is not expected to contribute anything that could be substituted (I don't see substitutions coming from file paths), I decided to opt out of `processLine()` entirely, and substitute `%{for-each-file}` as the last step.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150856