[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.


More information about the llvm-commits mailing list