Skip to content

Commit 19bc282

Browse files
authored
Add PR check to suggest alternatives to using undef (#118506)
As discussed in https://discourse.llvm.org/t/please-dont-use-undef-in-tests-part-2/83388, this patch adds a comment to PRs that introduce new uses of undef. It uses the the `git diff -S' command to find new uses, avoiding warning about old uses. It further trims down with a regex to get only added (+) lines.
1 parent a54fce8 commit 19bc282

File tree

1 file changed

+108
-1
lines changed

1 file changed

+108
-1
lines changed

llvm/utils/git/code-format-helper.py

Lines changed: 108 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@
1010

1111
import argparse
1212
import os
13+
import re
14+
import shlex
1315
import subprocess
1416
import sys
1517
from typing import List, Optional
@@ -312,7 +314,112 @@ def format_run(self, changed_files: List[str], args: FormatArgs) -> Optional[str
312314
return None
313315

314316

315-
ALL_FORMATTERS = (DarkerFormatHelper(), ClangFormatHelper())
317+
class UndefGetFormatHelper(FormatHelper):
318+
name = "undef deprecator"
319+
friendly_name = "undef deprecator"
320+
321+
@property
322+
def instructions(self) -> str:
323+
return " ".join(shlex.quote(c) for c in self.cmd)
324+
325+
def filter_changed_files(self, changed_files: List[str]) -> List[str]:
326+
filtered_files = []
327+
for path in changed_files:
328+
_, ext = os.path.splitext(path)
329+
if ext in (".cpp", ".c", ".h", ".hpp", ".hxx", ".cxx", ".inc", ".cppm", ".ll"):
330+
filtered_files.append(path)
331+
return filtered_files
332+
333+
def has_tool(self) -> bool:
334+
return True
335+
336+
def pr_comment_text_for_diff(self, diff: str) -> str:
337+
return f"""
338+
:warning: {self.name} found issues in your code. :warning:
339+
340+
<details>
341+
<summary>
342+
You can test this locally with the following command:
343+
</summary>
344+
345+
``````````bash
346+
{self.instructions}
347+
``````````
348+
349+
</details>
350+
351+
{diff}
352+
"""
353+
354+
def format_run(self, changed_files: List[str], args: FormatArgs) -> Optional[str]:
355+
files = self.filter_changed_files(changed_files)
356+
357+
# Use git to find files that have had a change in the number of undefs
358+
regex = "([^a-zA-Z0-9#_-]undef[^a-zA-Z0-9_-]|UndefValue::get)"
359+
cmd = ["git", "diff", "-U0", "--pickaxe-regex", "-S", regex]
360+
361+
if args.start_rev and args.end_rev:
362+
cmd.append(args.start_rev)
363+
cmd.append(args.end_rev)
364+
365+
cmd += files
366+
self.cmd = cmd
367+
368+
if args.verbose:
369+
print(f"Running: {self.instructions}")
370+
371+
proc = subprocess.run(
372+
cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, encoding="utf-8"
373+
)
374+
sys.stdout.write(proc.stderr)
375+
stdout = proc.stdout
376+
377+
files = []
378+
# Split the diff so we have one array entry per file.
379+
# Each file is prefixed like:
380+
# diff --git a/file b/file
381+
for file in re.split("^diff --git ", stdout, 0, re.MULTILINE):
382+
# search for additions of undef
383+
if re.search("^[+].*" + regex, file, re.MULTILINE):
384+
files.append(re.match("a/([^ ]+)", file.splitlines()[0])[1])
385+
386+
if not files:
387+
return None
388+
389+
files = "\n".join(" - " + f for f in files)
390+
report = f"""
391+
The following files introduce new uses of undef:
392+
{files}
393+
394+
[Undef](https://llvm.org/docs/LangRef.html#undefined-values) is now deprecated and should only be used in the rare cases where no replacement is possible. For example, a load of uninitialized memory yields `undef`. You should use `poison` values for placeholders instead.
395+
396+
In tests, avoid using `undef` and having tests that trigger undefined behavior. If you need an operand with some unimportant value, you can add a new argument to the function and use that instead.
397+
398+
For example, this is considered a bad practice:
399+
```llvm
400+
define void @fn() {{
401+
...
402+
br i1 undef, ...
403+
}}
404+
```
405+
406+
Please use the following instead:
407+
```llvm
408+
define void @fn(i1 %cond) {{
409+
...
410+
br i1 %cond, ...
411+
}}
412+
```
413+
414+
Please refer to the [Undefined Behavior Manual](https://llvm.org/docs/UndefinedBehavior.html) for more information.
415+
"""
416+
if args.verbose:
417+
print(f"error: {self.name} failed")
418+
print(report)
419+
return report
420+
421+
422+
ALL_FORMATTERS = (DarkerFormatHelper(), ClangFormatHelper(), UndefGetFormatHelper())
316423

317424

318425
def hook_main():

0 commit comments

Comments
 (0)