[PATCH] D83894: [lit] Don't expand escapes until all substitutions have been applied

[Louis Dionne via Phabricator via llvm-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/utils/lit/lit/TestRunner.py:1168
+
+            if ignore_double_percent_substitution and a == '#_MARKER_#':
+                # Defer the %% substitution until the last pass when
----------------
I was working on a similar change locally, and the approach I took instead was this:

1. Remove both `('%%', '#_MARKER_#')` and `('#_MARKER_#', '%')` from the default substitutions, which is done above in `getDefaultSubstitutions`.
2. Define the following functions in `applySubstitutions`: 
```
def escape(ln):
    return _caching_re_compile('%%').sub('#_MARKER_#', ln)

def unescape(ln):
    return _caching_re_compile('#_MARKER_#').sub('%', ln)
```

3. Apply those functions around the actual substitution: `return list(map(lambda ln: unescape(process(escape(ln))), script))`.

This approach seems simpler to me and I think it's a bit more explicit in what we're trying to achieve. It also has the added benefit that we don't clutter the list of substitutions with those substitutions that are implementation details of the escape mechanism only.

WDYT?



================
Comment at: llvm/utils/lit/tests/shtest-recursive-substitution.py:23
 # RUN: %{lit} -j 1 %{inputs}/shtest-recursive-substitution/set-to-none --show-all | FileCheck --check-prefix=CHECK-TEST6 %s
 # CHECK-TEST6: PASS: set-to-none :: test.py
----------------
Can you add a new test case instead of piggy-backing on the existing ones? It seems cleaner to me to test one correctness aspect for each test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83894