libeigen/eigen!2775 Co-authored-by: Rasmus Munk Larsen <rmlarsen@gmail.com>
398 lines
18 KiB
Python
398 lines
18 KiB
Python
#!/usr/bin/env python3
|
|
# SPDX-FileCopyrightText: The Eigen Authors
|
|
# SPDX-License-Identifier: MPL-2.0
|
|
|
|
"""Advisory style check for added C++ code, for what clang-tidy cannot say.
|
|
|
|
Flags the problems that recur in this repository's code reviews and that no
|
|
clang-tidy check states: narration-comment verbosity (see the comment rules in
|
|
``AGENTS.md``), ``enum`` constant blocks, ``std::integral_constant<bool,...>``,
|
|
C++17-and-later constructs in the C++14 trees, and ``std::`` math in library
|
|
headers where ``numext::`` is required. Conventions clang-tidy *can* state
|
|
live in ``.clang-tidy`` and are checked by ``clang_tidy_hook.py`` against the
|
|
same added lines. Only lines a change ADDS are reported, but each file's
|
|
complete post-image is lexed so surrounding context — an enclosing block
|
|
comment, a Doxygen continuation, a multi-line initializer — is classified
|
|
correctly.
|
|
|
|
The findings are advisory: a flagged construct may be justified, in which
|
|
case keep it and state the reason where the construct is.
|
|
|
|
Modes:
|
|
--diff BASE check lines added relative to ``merge-base(BASE, HEAD)``,
|
|
including uncommitted changes and untracked C++ files;
|
|
exit 1 if findings
|
|
--claude-hook run as a Claude Code PostToolUse hook: read the tool-call
|
|
JSON from stdin and check the text the edit added; exit 2
|
|
if findings so the harness feeds them back to the model
|
|
|
|
The Claude Code hook is registered for this repository in
|
|
``.claude/settings.json``. Other agents and humans can run the diff mode
|
|
directly, e.g. ``python3 scripts/check_style.py --diff origin/master``.
|
|
"""
|
|
|
|
import argparse
|
|
import json
|
|
import os
|
|
import re
|
|
import sys
|
|
|
|
sys.path.insert(0, os.path.dirname(os.path.abspath(__file__)))
|
|
|
|
from style_common import (REPO_ROOT, added_lines_from_diff, added_from_structured_patch, # noqa: E402,F401
|
|
diff_added_lines, hook_added_line_numbers, hook_edit_snippets,
|
|
hook_post_image_and_added, is_cxx_path, read_post_image)
|
|
|
|
# Trees whose headers and tests must compile as C++14 (see AGENTS.md rule 3).
|
|
CXX14_TREES = ("Eigen/", "unsupported/Eigen/", "test/", "unsupported/test/", "failtest/", "blas/", "lapack/")
|
|
# Guarded exceptions with a documented newer requirement: SYCL sources build
|
|
# only in the C++17 SYCL configurations, while duccfft and the structured-
|
|
# binding failtests explicitly select C++17 in their test CMakeLists files.
|
|
CXX17_PREFIXES = ("Eigen/src/Core/arch/SYCL/",)
|
|
CXX17_FILES = {
|
|
"test/sycl_basic.cpp",
|
|
"unsupported/Eigen/src/FFT/duccfft_impl.h",
|
|
"unsupported/test/duccfft.cpp",
|
|
"failtest/structured_bindings_dynamic_matrix.cpp",
|
|
"failtest/structured_bindings_dynamic_array.cpp",
|
|
"failtest/structured_bindings_rowmajor.cpp",
|
|
}
|
|
# Library implementation headers, where numext:: is required over std:: math.
|
|
LIBRARY_SRC_TREES = ("Eigen/src/", "unsupported/Eigen/src/")
|
|
|
|
DOXYGEN = re.compile(r"^\s*(/\*\*|/\*!|///|//!)")
|
|
LICENSE = re.compile(r"SPDX|Copyright|License|Mozilla Public", re.I)
|
|
STD_MATH = (
|
|
r"abs|arg|conj|real|imag|sqrt|cbrt|isnan|isinf|isfinite|signbit|exp|exp2|expm1|log|log1p|log2|pow|"
|
|
r"sin|cos|tan|asin|acos|atan|atan2|asinh|acosh|atanh|sinh|cosh|tanh|hypot|fma|copysign|ldexp|floor|"
|
|
r"ceil|round|rint|trunc|fmod"
|
|
)
|
|
|
|
# Convention checks applied per added code line (comments and literals blanked).
|
|
#
|
|
# Only conventions clang-tidy cannot currently state belong here; NULL and
|
|
# typedef moved to modernize-use-nullptr and modernize-use-using in
|
|
# .clang-tidy, which clang_tidy_hook.py and ci/scripts/run-clang-tidy.sh run
|
|
# against the same added lines. The entries below are the remainder: those
|
|
# needing a CustomChecks query (parked in .clang-tidy until CI's clang-tidy
|
|
# reaches 22) and those no AST matcher reaches at all.
|
|
#
|
|
# One known gap in that handover: modernize-use-using in clang-tidy 18 (the
|
|
# checkformat:clangtidy toolchain) reports typedefs at class and namespace
|
|
# scope but not function-local ones (measured — Block.h 17 of 18, Redux.h 24
|
|
# of 25, GeneralMatrixMatrix.h 21 of 28). That was an upstream defect,
|
|
# llvm/llvm-project#72179, fixed in the 19.x release, so the gap closes when
|
|
# the CI image's clang-tidy moves; until then a function-local typedef in new
|
|
# code goes unreported. Reintroducing a regex for it would bring back the
|
|
# false positives this handover removes. modernize-use-nullptr has no gap.
|
|
CODE_CHECKS = [
|
|
# Awaiting the parked eigen-bool-constant query.
|
|
(r"std::integral_constant<\s*bool\b", "std::integral_constant<bool,...>: use Eigen's bool_constant (Meta.h)"),
|
|
# Awaiting the parked eigen-enum-constant query. No stock check states
|
|
# this rule: cppcoreguidelines-use-enum-class asks for `enum class`,
|
|
# whereas Eigen wants static constexpr for trait constants.
|
|
(r"^\s*enum\s*(?::[^{}]+)?\{", "enum constant block: trait/evaluator constants are static constexpr in new "
|
|
"code; Flags is `unsigned int`"),
|
|
]
|
|
CXX14_CHECKS = [
|
|
# Awaiting the parked eigen-if-constexpr query. A C++14 build does not
|
|
# reliably reject this on its own: clang accepts `if constexpr` as an
|
|
# extension and only warns under -Wc++17-extensions.
|
|
(r"\bif\s+constexpr\b", "if constexpr: supported code compiles as C++14; use EIGEN_IF_CONSTEXPR (...) with a "
|
|
"condition that is valid either way"),
|
|
# These do fail the default C++14 build; flagging them at edit time just
|
|
# shortens the loop, and covers headers no built target happens to include.
|
|
(r"\bstd::(any|span|optional|variant|string_view|byte|filesystem|void_t)\b",
|
|
"C++17/20 library facility: the supported baseline is C++14"),
|
|
]
|
|
# Also C++14-tree only (CMakeLists.txt defaults CMAKE_CXX_STANDARD to 14), and
|
|
# not expressible as a matcher — designated initializers do not parse in C++14,
|
|
# so there is no AST for a check to match.
|
|
DESIGNATED_MSG = "designated initializer: C++20-only; use aggregate assignment with /*name=*/ comments"
|
|
|
|
|
|
RAW_PREFIX = re.compile(r"(?:^|[^0-9A-Za-z_])(?:u8|u|U|L)?R$")
|
|
|
|
|
|
def is_digit_separator(line, i):
|
|
"""Return whether the apostrophe at ``i`` belongs to a C++ number."""
|
|
if i + 1 >= len(line) or not line[i + 1].isalnum():
|
|
return False
|
|
start = i
|
|
while start > 0 and (line[start - 1].isalnum() or line[start - 1] in "._'"):
|
|
start -= 1
|
|
token = line[start:i]
|
|
return bool(token) and (token[0].isdigit() or (len(token) > 1 and token[0] == "." and token[1].isdigit()))
|
|
|
|
|
|
def scan_quoted(line, j, quote):
|
|
"""Scan a quoted literal's remainder from position ``j``.
|
|
|
|
Returns (next_position, closed, spliced): ``spliced`` is True when the
|
|
line ends with a backslash inside the literal, so the literal continues
|
|
on the next physical line.
|
|
"""
|
|
n = len(line)
|
|
while j < n:
|
|
if line[j] == "\\":
|
|
if j == n - 1:
|
|
return n, False, True
|
|
j += 2
|
|
continue
|
|
if line[j] == quote:
|
|
return j + 1, True, False
|
|
j += 1
|
|
return n, False, False
|
|
|
|
|
|
def lex_lines(lines):
|
|
"""Lex a file's complete contiguous text.
|
|
|
|
Returns (code_lines, comment_only, doxygen_only) as parallel lists: per
|
|
line, the code with comments and string/character literals blanked, the
|
|
stripped comment text when the line holds no code (else None), and whether
|
|
that comment belongs to Doxygen. Block comments, raw string literals, and
|
|
backslash-spliced ordinary literals carry their state across physical
|
|
lines.
|
|
"""
|
|
code_lines, comment_only, doxygen_only = [], [], []
|
|
in_block = False
|
|
block_doxygen = False
|
|
raw_terminator = None # e.g. )delim" while inside a raw string literal
|
|
open_quote = None # quote character of a spliced ordinary literal
|
|
for line in lines:
|
|
out, i, n = [], 0, len(line)
|
|
had_code = False
|
|
had_comment = in_block
|
|
line_doxygen = block_doxygen if in_block else False
|
|
literal_at_start = raw_terminator is not None or open_quote is not None
|
|
while i < n:
|
|
if in_block:
|
|
j = line.find("*/", i)
|
|
if j < 0:
|
|
i = n
|
|
else:
|
|
in_block = False
|
|
block_doxygen = False
|
|
i = j + 2
|
|
continue
|
|
if raw_terminator is not None:
|
|
j = line.find(raw_terminator, i)
|
|
if j < 0:
|
|
i = n
|
|
else:
|
|
i = j + len(raw_terminator)
|
|
raw_terminator = None
|
|
continue
|
|
if open_quote is not None:
|
|
i, closed, spliced = scan_quoted(line, i, open_quote)
|
|
if closed or not spliced: # an unspliced open literal is ill-formed; close at EOL
|
|
open_quote = None
|
|
continue
|
|
c = line[i]
|
|
if c == "/" and i + 1 < n and line[i + 1] == "/":
|
|
had_comment = True
|
|
line_doxygen = line_doxygen or bool(DOXYGEN.match(line[i:]))
|
|
break
|
|
if c == "/" and i + 1 < n and line[i + 1] == "*":
|
|
had_comment = True
|
|
block_doxygen = line.startswith(("/**", "/*!"), i)
|
|
line_doxygen = line_doxygen or block_doxygen
|
|
in_block = True
|
|
i += 2
|
|
continue
|
|
if c == '"' and RAW_PREFIX.search(line[:i]):
|
|
paren = line.find("(", i + 1)
|
|
delimiter = line[i + 1:paren] if paren >= 0 else None
|
|
if delimiter is not None and len(delimiter) <= 16 and not re.search(r"[()\\\s]", delimiter):
|
|
out.append('""')
|
|
had_code = True
|
|
raw_terminator = ")" + delimiter + '"'
|
|
j = line.find(raw_terminator, paren + 1)
|
|
if j < 0:
|
|
i = n
|
|
else:
|
|
i = j + len(raw_terminator)
|
|
raw_terminator = None
|
|
continue
|
|
if c == "'" and is_digit_separator(line, i):
|
|
# C++14 digit separator inside a preprocessing-number, not a
|
|
# character-literal delimiter (for example 1'000 or 0xFF'00).
|
|
out.append(c)
|
|
had_code = True
|
|
i += 1
|
|
continue
|
|
if c in "\"'":
|
|
out.append(c + c)
|
|
had_code = True
|
|
i, closed, spliced = scan_quoted(line, i + 1, c)
|
|
if not closed and spliced:
|
|
open_quote = c
|
|
continue
|
|
out.append(c)
|
|
if not c.isspace():
|
|
had_code = True
|
|
i += 1
|
|
stripped = line.strip()
|
|
is_comment = (not had_code) and had_comment and not literal_at_start
|
|
code_lines.append("".join(out))
|
|
comment_only.append(stripped if is_comment else None)
|
|
doxygen_only.append(line_doxygen if is_comment else None)
|
|
return code_lines, comment_only, doxygen_only
|
|
|
|
|
|
def check_comments(comment_only, doxygen_only, added, findings):
|
|
"""Flag comment blocks that gain six or more added narration lines.
|
|
|
|
Blocks are formed over the full file, so an addition inside an existing
|
|
Doxygen or license block inherits that block's exemption, and only the
|
|
ADDED lines count toward the threshold — extending a pre-existing block
|
|
by a line or two is not reported.
|
|
"""
|
|
def finish_block(block_start, block_end, is_doxygen):
|
|
block = comment_only[block_start:block_end]
|
|
added_in_block = [line_no for line_no in range(block_start + 1, block_end + 1) if line_no in added]
|
|
if added_in_block and len(added_in_block) >= 6 and not (is_doxygen or LICENSE.search("\n".join(block))):
|
|
findings.append((added_in_block[0], "%d added lines of non-Doxygen comment: AGENTS.md keeps only "
|
|
"mathematics, invariants, compatibility constraints, provenance, "
|
|
"or the reason a deliberate construct must not be simplified"
|
|
% len(added_in_block)))
|
|
|
|
block_start, block_doxygen = None, None
|
|
for idx in range(len(comment_only) + 1):
|
|
comment = comment_only[idx] if idx < len(comment_only) else None
|
|
if comment is not None:
|
|
if block_start is None:
|
|
block_start = idx
|
|
block_doxygen = doxygen_only[idx]
|
|
elif doxygen_only[idx] != block_doxygen:
|
|
finish_block(block_start, idx, block_doxygen)
|
|
block_start = idx
|
|
block_doxygen = doxygen_only[idx]
|
|
elif block_start is not None:
|
|
finish_block(block_start, idx, block_doxygen)
|
|
block_start = None
|
|
|
|
|
|
def check_designated_initializer(code_lines, line_no, findings):
|
|
code = code_lines[line_no - 1]
|
|
hit = re.search(r"[{,]\s*\.\w+\s*=", code)
|
|
if not hit and re.match(r"\s*\.\w+\s*=", code):
|
|
# A line-leading designator continues a braced initializer only when
|
|
# the previous code line ends with `{` or `,`; anything else is a
|
|
# wrapped member access or assignment.
|
|
for prev in range(line_no - 2, -1, -1):
|
|
prev_code = code_lines[prev].rstrip()
|
|
if prev_code:
|
|
hit = prev_code.endswith("{") or prev_code.endswith(",")
|
|
break
|
|
if hit:
|
|
findings.append((line_no, DESIGNATED_MSG))
|
|
return True
|
|
return False
|
|
|
|
|
|
def check_conventions(rel_path, code_lines, added, findings):
|
|
checks = list(CODE_CHECKS)
|
|
cxx17_path = (
|
|
rel_path in CXX17_FILES
|
|
or rel_path.startswith(CXX17_PREFIXES)
|
|
or (rel_path.startswith("unsupported/Eigen/src/Tensor/") and rel_path.endswith("Sycl.h"))
|
|
or (rel_path.startswith("unsupported/test/") and rel_path.endswith("_sycl.cpp"))
|
|
)
|
|
if rel_path.startswith(CXX14_TREES) and not cxx17_path:
|
|
checks += CXX14_CHECKS
|
|
if rel_path.startswith(LIBRARY_SRC_TREES):
|
|
checks.append((r"(?:\(\s*)?\bstd::(?:%s)\s*(?:\)\s*)?\(" % STD_MATH,
|
|
"std:: math call in a library header: use the numext:: equivalent "
|
|
"(device- and custom-scalar-aware)"))
|
|
checks.append((r"(?:\(\s*)?\bstd::frexp\s*(?:\)\s*)?\(",
|
|
"std::frexp call in a library header: use EIGEN_USING_STD(frexp) and an unqualified call "
|
|
"for ADL and device/custom-scalar support"))
|
|
added_sorted = sorted(added)
|
|
for pattern, message in checks:
|
|
rx = re.compile(pattern)
|
|
for line_no in added_sorted:
|
|
if rx.search(code_lines[line_no - 1]):
|
|
findings.append((line_no, message))
|
|
break # one report per pattern per file
|
|
for line_no in added_sorted:
|
|
if check_designated_initializer(code_lines, line_no, findings):
|
|
break
|
|
|
|
|
|
def find_findings(rel_path, lines, added):
|
|
"""Check one file: ``lines`` is the complete post-image, ``added`` the set
|
|
of 1-based line numbers the change added. Returns [(line_no, message)]."""
|
|
if not is_cxx_path(rel_path):
|
|
return []
|
|
added = {n for n in added if 1 <= n <= len(lines) and lines[n - 1].strip()}
|
|
if not added:
|
|
return []
|
|
code_lines, comment_only, doxygen_only = lex_lines(lines)
|
|
findings = []
|
|
check_comments(comment_only, doxygen_only, added, findings)
|
|
check_conventions(rel_path, code_lines, added, findings)
|
|
findings.sort()
|
|
return findings
|
|
|
|
|
|
def run_diff_mode(base, root=REPO_ROOT):
|
|
"""Return [(rel_path, line_no, message)] for lines added since
|
|
merge-base(base, HEAD), including untracked C++ files."""
|
|
per_file = diff_added_lines(base, root)
|
|
results = []
|
|
for rel_path in sorted(per_file):
|
|
text = read_post_image(root, rel_path)
|
|
if text is None: # deleted or unreadable
|
|
continue
|
|
for line_no, message in find_findings(rel_path, text.splitlines(), per_file[rel_path]):
|
|
results.append((rel_path, line_no, message))
|
|
return results
|
|
|
|
|
|
def run_hook_mode():
|
|
try:
|
|
payload = json.load(sys.stdin)
|
|
except Exception:
|
|
return 0
|
|
rel_path, snippets = hook_edit_snippets(payload)
|
|
if rel_path is None:
|
|
return 0
|
|
# The hook runs after the edit, so the file on disk is the post-image; it is
|
|
# lexed whole so enclosing comments and initializers classify correctly.
|
|
lines, added, _ = hook_post_image_and_added(payload, rel_path, snippets)
|
|
findings = find_findings(rel_path, lines, added)
|
|
if findings:
|
|
sys.stderr.write("style check (%s) — review before proceeding:\n" % rel_path)
|
|
for _, message in findings[:8]:
|
|
sys.stderr.write(" - %s\n" % message)
|
|
sys.stderr.write("Advisory: keep a flagged construct only if it is justified, and say why at the "
|
|
"construct.\n")
|
|
return 2
|
|
return 0
|
|
|
|
|
|
def main():
|
|
parser = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter)
|
|
mode = parser.add_mutually_exclusive_group(required=True)
|
|
mode.add_argument("--diff", metavar="BASE", help="check lines added relative to merge-base(BASE, HEAD)")
|
|
mode.add_argument("--claude-hook", action="store_true", help="run as a Claude Code PostToolUse hook")
|
|
args = parser.parse_args()
|
|
if args.claude_hook:
|
|
try:
|
|
return run_hook_mode()
|
|
except Exception:
|
|
return 0 # a broken hook must not block the harness
|
|
results = run_diff_mode(args.diff)
|
|
for rel_path, line_no, message in results:
|
|
print("%s:%d: %s" % (rel_path, line_no, message))
|
|
if results:
|
|
print("\n%d advisory finding(s) in added lines. Keep a flagged construct only if it is justified, "
|
|
"and say why at the construct." % len(results))
|
|
return 1 if results else 0
|
|
|
|
|
|
if __name__ == "__main__":
|
|
sys.exit(main())
|