[llvm] 19bc282 - Add PR check to suggest alternatives to using undef (#118506)

[via llvm-commits]

Author: Nuno Lopes
Date: 2024-12-11T21:09:05Z
New Revision: 19bc282320ba4d2e961e287f110b9110297ae3ee

URL: https://github.com/llvm/llvm-project/commit/19bc282320ba4d2e961e287f110b9110297ae3ee
DIFF: https://github.com/llvm/llvm-project/commit/19bc282320ba4d2e961e287f110b9110297ae3ee.diff

LOG: 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.

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/llvm/utils/git/code-format-helper.py b/llvm/utils/git/code-format-helper.py
index 76b2a3e26be28a..19264bca6ce8f6 100755
--- a/llvm/utils/git/code-format-helper.py
+++ b/llvm/utils/git/code-format-helper.py
@@ -10,6 +10,8 @@
 
 import argparse
 import os
+import re
+import shlex
 import subprocess
 import sys
 from typing import List, Optional
@@ -312,7 +314,112 @@ def format_run(self, changed_files: List[str], args: FormatArgs) -> Optional[str
             return None
 
 
-ALL_FORMATTERS = (DarkerFormatHelper(), ClangFormatHelper())
+class UndefGetFormatHelper(FormatHelper):
+    name = "undef deprecator"
+    friendly_name = "undef deprecator"
+
+    @property
+    def instructions(self) -> str:
+        return " ".join(shlex.quote(c) for c in self.cmd)
+
+    def filter_changed_files(self, changed_files: List[str]) -> List[str]:
+        filtered_files = []
+        for path in changed_files:
+            _, ext = os.path.splitext(path)
+            if ext in (".cpp", ".c", ".h", ".hpp", ".hxx", ".cxx", ".inc", ".cppm", ".ll"):
+                filtered_files.append(path)
+        return filtered_files
+
+    def has_tool(self) -> bool:
+        return True
+
+    def pr_comment_text_for_
diff (self, 
diff : str) -> str:
+        return f"""
+:warning: {self.name} found issues in your code. :warning:
+
+<details>
+<summary>
+You can test this locally with the following command:
+</summary>
+
+``````````bash
+{self.instructions}
+``````````
+
+</details>
+
+{
diff }
+"""
+
+    def format_run(self, changed_files: List[str], args: FormatArgs) -> Optional[str]:
+        files = self.filter_changed_files(changed_files)
+
+        # Use git to find files that have had a change in the number of undefs
+        regex = "([^a-zA-Z0-9#_-]undef[^a-zA-Z0-9_-]|UndefValue::get)"
+        cmd = ["git", "
diff ", "-U0", "--pickaxe-regex", "-S", regex]
+
+        if args.start_rev and args.end_rev:
+            cmd.append(args.start_rev)
+            cmd.append(args.end_rev)
+
+        cmd += files
+        self.cmd = cmd
+
+        if args.verbose:
+            print(f"Running: {self.instructions}")
+
+        proc = subprocess.run(
+            cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, encoding="utf-8"
+        )
+        sys.stdout.write(proc.stderr)
+        stdout = proc.stdout
+
+        files = []
+        # Split the 
diff  so we have one array entry per file.
+        # Each file is prefixed like:
+        # 
diff  --git a/file b/file
+        for file in re.split("^
diff  --git ", stdout, 0, re.MULTILINE):
+            # search for additions of undef
+            if re.search("^[+].*" + regex, file, re.MULTILINE):
+                files.append(re.match("a/([^ ]+)", file.splitlines()[0])[1])
+
+        if not files:
+            return None
+
+        files = "\n".join(" - " + f for f in files)
+        report = f"""
+The following files introduce new uses of undef:
+{files}
+
+[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.
+
+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.
+
+For example, this is considered a bad practice:
+```llvm
+define void @fn() {{
+  ...
+  br i1 undef, ...
+}}
+```
+
+Please use the following instead:
+```llvm
+define void @fn(i1 %cond) {{
+  ...
+  br i1 %cond, ...
+}}
+```
+
+Please refer to the [Undefined Behavior Manual](https://llvm.org/docs/UndefinedBehavior.html) for more information.
+"""
+        if args.verbose:
+            print(f"error: {self.name} failed")
+            print(report)
+        return report
+
+
+ALL_FORMATTERS = (DarkerFormatHelper(), ClangFormatHelper(), UndefGetFormatHelper())
 
 
 def hook_main():