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

Vlad Serebrennikov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sat May 20 00:29:48 PDT 2023

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.

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

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.

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list