[llvm] [lit] Fix substitutions containing backslashes (PR #103042)

Martin Storsjö via llvm-commits llvm-commits at lists.llvm.org
Tue Aug 20 14:37:56 PDT 2024


mstorsjo wrote:

> > Script provided substitutions containing backslashes did not work on Unix, but they did work on Windows.
> 
> That depends on the definition of "work". In downstream test suites (in lit config scripts but obviously not (RE)DEFINE), I have made use of replacement string backslashes (e.g., `\g`) as defined by `re.sub`. I doubt that specific work will ever go upstream, but it makes me wonder if we should advertise this change in behavior more broadly.

Oh, that's interesting to know - especially as this case actually seems to have been documented to do this.

> > This lead to a Windows specific escaping of backslashes before doing Python re substitutions - since [7c9eab8](https://github.com/llvm/llvm-project/commit/7c9eab8fef0ed79a5911d21eb97b6b0fa9d39f82).
> 
> I think that means my usage above cannot behave correctly in Windows, where I never tested it. With that in mind, my estimation is that backslash escaping is the desired and safest behavior in the vast majority of cases (probably all upstream cases). In the future if needed, we could surely find some per-substitution switch to opt-in to full `re.sub` behavior.

Thanks! Indeed, this is probably the behaviour that is most convenient for most uses, but as you say, it may be good to be able to use the real escape sequences as well somehow.

> Please update the first bullet under "Test-specific substitutions" in https://llvm.org/docs/TestingGuide.html#substitutions as it advertises `re.sub` behavior.

Ok, I updated the PR with that docs change. This does indeed highlight that this is, in principle a breaking change wrt the docs, even if no cases upstream did exercise this (and it wasn't working as intended on Windows).

> It might also be good to have a test case showing that lit config script substitutions properly handle escape sequences. You're already updating the one for (RE)DEFINE.

Done - I added a case for that. I wasn't sure which test suite would be best to have (there didn't seem to be any subdirectory specifically for testing general substitutions), so I stuck it in `shtest-define` next to the other test - feel free to suggest other better places for it.

https://github.com/llvm/llvm-project/pull/103042


More information about the llvm-commits mailing list