[llvm] [lit] Fix some issues from --per-test-coverage (PR #65242)
Joel E. Denny via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 12 08:52:00 PDT 2023
================
@@ -1837,13 +1830,7 @@ def _handleCommand(cls, line_number, line, output, keyword):
if not output or not output[-1].add_continuation(line_number, keyword, line):
if output is None:
output = []
- pdbg = "%dbg({keyword} at line {line_number})".format(
- keyword=keyword, line_number=line_number
- )
- assert re.match(
- kPdbgRegex + "$", pdbg
----------------
jdenny-ornl wrote:
Good question.
Originally, my goal was for the assert to mimic how kPdbgRegex will actually be used later when expanding `%dbg` substitutions. That's the usage that the assert is trying to verify will work correctly. Thus, this patch changes the assert in two ways: the searched string contains all of `%dbg(...) cmd-line` instead of just `%dbg(...)`, and it does not use `$`.
However, now that you ask, I think the assert should be stricter. For example, [D154987](https://reviews.llvm.org/D154987) proposes to extend kPdbgRegex to permit newlines, which might appear in `lit.run(cmd)` in PYTHON directives, as in the example in that review's summary. Using `$` in the assert here would have caught that existing deficiency of kPdbgRegex: its `.*` doesn't match beyond the first newline.
Instead of `$`, what do you think of using `re.fullmatch` in the assert? That is, buildPdbgCommand expects kPdbgRegex to match the entire string it's building.
https://github.com/llvm/llvm-project/pull/65242
More information about the llvm-commits
mailing list