From 1fea9d4b33fea829373577ad4d898f29ab8c9431 Mon Sep 17 00:00:00 2001 From: dayfine Date: Mon, 9 Nov 2026 22:37:06 -0800 Subject: [PATCH] Code health: ban char*, prefix overloads, and too many header names Three rules from the review of #312: no char*/char[] types on added lines, no header overload pair whose parameters are a strict prefix of the other's (use a default argument or std::optional), and at most 8 public namespace-scope names per header (7-8 advisory, base comparison like public methods). The two style rules take a justified allow tag on or above the flagged line; header_symbols takes a file-level tag. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_013VUubhHeYSMGSSDXZkKZss --- AGENTS.md | 12 +- devtools/code_health/README.md | 10 +- devtools/code_health/__main__.py | 9 +- devtools/code_health/allow.py | 41 +++- devtools/code_health/config.py | 8 +- devtools/code_health/findings.py | 1 + devtools/code_health/header_api.py | 149 +++++++++++++ devtools/code_health/style.py | 21 ++ devtools/code_health/tests/test_cli.py | 16 ++ devtools/code_health/tests/test_header_api.py | 196 ++++++++++++++++++ devtools/code_health/tests/test_style.py | 2 +- 11 files changed, 452 insertions(+), 13 deletions(-) create mode 100644 devtools/code_health/header_api.py create mode 100644 devtools/code_health/tests/test_header_api.py diff --git a/AGENTS.md b/AGENTS.md index b28d729447..b1a71e631d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -314,6 +314,9 @@ so keep it where it clearly pays off. `std::string_view` is the exception (see S ids (`for (std::string_view slug : {...})`). Never store it past the call (TotW #1, #180). The user's call over upstream's `const std::string&` habit (2026-11-02): fix it across our PRs before an upstream handoff. Existing upstream APIs we don't otherwise touch keep their signature. +- **No `char*` / `char[]` in new code.** Use `std::string`, or `std::string_view` + (`inline constexpr std::string_view` for constants); `unsigned char` bytes and `main(argc, argv)` + are fine (the user, review of #312). Code Health fails added lines. - **`std::format`** for messages with several parts, instead of `+` / `std::to_string` chains or `ostringstream` (TotW #3, #124). Raw string literals for embedded JSON (TotW #64). @@ -355,12 +358,19 @@ so keep it where it clearly pays off. `std::string_view` is the exception (see S `/*save=*/false` at the call site. - **No sentinel values in new APIs** (`-1`, `""`, `0` meaning "none"). Return `std::optional` or a result enum like `WildBattleExitResult` (TotW #171). For an optional - input, overload or take `const T*`, not `const std::optional&` or `= ""` (TotW #163). + input, use a default argument with a real value, `std::optional` by value or `const T*`, + not `const std::optional&` or `= ""` (TotW #163). - **No top-level `const` on by-value parameters in declarations** (TotW #109). - **File-local helpers go in an unnamed `namespace{`**, not `static` and not the header (TotW #186). - **Overloads only when every overload means the same thing** (`move_cursor_to_option(..., BattleMenuOption / BattleMoveOption)`) (TotW #148). +- **Default arguments over overloads.** When one overload's parameters are the other's plus more, + write one function with a default argument or a `std::optional` parameter (the user, review of + #312). Code Health fails such a pair in a header when either is new. +- **Few names per header:** at most 8 public namespace-scope names (types, aliases, free functions + with their overloads, constants); 7-8 is a hint to split. `detail`/anonymous namespaces and class + members don't count. Code Health holds new headers to 8 and existing ones to their base count. - **Lambdas, not `std::bind`** (TotW #108). Free functions over extra public methods (TotW #99). diff --git a/devtools/code_health/README.md b/devtools/code_health/README.md index 50863dd7e3..a5be400e49 100644 --- a/devtools/code_health/README.md +++ b/devtools/code_health/README.md @@ -49,6 +49,7 @@ are included. Missing base revisions and analysis errors fail the command. | Public methods per class/struct | 7 (excluding ctors, dtors, operators, deleted and Qt signals); 6-7 advisory | | Public non-static data in a class with mutating methods | 0 (exempt: plain structs, value types with only const/static methods, const fields, `*Option`/`*Cell` fields, test files) | | Single-method interface (one pure virtual + optional virtual dtor, no bases) | not allowed: prefer a callable (`MoveOnlyFunction` / `std::function`) | +| Public names per header: distinct namespace-scope types, enums, aliases, free functions (overloads count once) and constants/variables, outside anonymous, `detail`, `internal` and `*Internal` namespaces; class members, operators, forward declarations and macros excluded (`header_api.py`) | 8; 7-8 advisory | New files/functions must meet these limits. In modified files, compare each metric with its value at the base revision: existing violations may remain or @@ -94,11 +95,14 @@ void load_move_table(){ `// code-health: allow [,] -- ` lets the listed metrics of one function or class exceed their limit. Checks are the metric names in the table above: `file_lines`, `function_lines`, `nesting`, `parameters`, `public_methods`, `public_data`, -`single_method_interface`. Style and layering findings have no bypass. +`single_method_interface`, `header_symbols`. Two style findings take a tag too: `char_pointer` and +`overload_prefix`, for a C API or a fixed interface that needs them. Other style and layering +findings have no bypass. - **Place it** on the function's or class's first line, or in the comment block directly above it (only `//` comment and `template` lines between). A `file_lines` tag goes in the first 20 - lines of the file. A tag that attaches to nothing fails. + lines of the file, and so does a `header_symbols` tag. A `char_pointer` or `overload_prefix` tag goes on + the flagged line or in the comment block directly above it. A tag that attaches to nothing fails. - **Justify it:** the text after `--`, on the tag line itself, must be at least 40 characters and 6 words, and say why splitting would make the code worse. Unknown checks, a missing or short justification, or a malformed tag fail. @@ -148,6 +152,8 @@ lexer, not a parser: rules that need types stay in review. | A defaulted `bool` parameter, or 2+ `bool` parameters (not `override`/`final`, not an unchanged signature) | error | TotW #94 | | `using namespace` at namespace scope in a header | error | TotW #153 | | Forward `class`/`struct` declaration in a header: new header / existing header | error / advisory | fork AGENTS.md "Forward declarations" | +| `char*` / `const char*` / `char**` types and named `char` arrays (`char buf[8]`); not `unsigned`/`signed char`, `char8_t`-`char32_t`, `wchar_t`, a plain `char`, `*_cast`, or `main(argc, argv)` | error (tag: `char_pointer`) | fork AGENTS.md "Strings" | +| In a header, two declarations of one function in the same scope (namespace or class) whose parameter types are a strict prefix of each other, one of them on an added line; not constructors, operators, `= delete`, variadic packs, or overloads that differ in a parameter's type | error (tag: `overload_prefix`) | fork AGENTS.md "Functions" | | A comment whose first sentence describes the neighbouring function, none of its own | advisory | CONTRIBUTING; Google "Function Comments" | Advisories print as `Advisory:` lines and `::warning` annotations and never fail. diff --git a/devtools/code_health/__main__.py b/devtools/code_health/__main__.py index 5fb612058f..7e24a3f2d5 100644 --- a/devtools/code_health/__main__.py +++ b/devtools/code_health/__main__.py @@ -5,7 +5,7 @@ import sys from pathlib import Path -from . import allow, layering, style +from . import allow, header_api, layering, style from .classes import measure_classes from .findings import annotations from .git_files import changed_files, default_base, read_revision @@ -14,7 +14,7 @@ def analyze(path, source): - return measure(path, source) + measure_classes(path, source) + return measure(path, source) + measure_classes(path, source) + header_api.measure_header(path, source) def run(root, base_ref=None, github=False): @@ -39,7 +39,10 @@ def run(root, base_ref=None, github=False): bypassed.extend(notices) notes.extend(unneeded) style_errors, style_notes = style.check(current, source, previous_source) - failures.extend(style_errors) + style_errors += header_api.prefix_overloads(current, source, previous_source) + style_errors, waived, unattached = allow.waive(current, source, allowances, style_errors) + failures.extend(style_errors + unattached) + bypassed.extend(waived) notes.extend(style_notes) advisory += sum(m.check == 'file_lines' and 500 < m.value <= 1000 for m in metrics) print(f'Scanned {len(changes)} changed C++ files against {base_ref}.') diff --git a/devtools/code_health/allow.py b/devtools/code_health/allow.py index fa12e3ecd0..d69aef523b 100644 --- a/devtools/code_health/allow.py +++ b/devtools/code_health/allow.py @@ -2,15 +2,22 @@ A tag on a function's or class's first line, or in the comment block directly above it, lets that function's or class's listed metrics exceed their limit up to a hard ceiling. A -`file_lines` tag goes in the first FILE_TAG_LINES lines. Every bypass is printed, quoting +`file_lines` or `header_symbols` tag goes in the first FILE_TAG_LINES lines. A `char_pointer` or +`overload_prefix` tag waives that style finding on its own line or the line below its comment +block. Every bypass is printed, quoting its justification, so reviewers see it; malformed, unknown or misplaced tags fail.""" import re from dataclasses import dataclass -from .config import CLASS_LIMITS +from .config import CLASS_LIMITS, HEADER_LIMITS from .findings import Finding -CHECKS = ('file_lines', 'function_lines', 'nesting', 'parameters') + tuple(CLASS_LIMITS) +# Style findings a tag can bypass; they attach like a function's tag, to the finding's line. +STYLE_CHECKS = ('char_pointer', 'overload_prefix') +# File-level metrics: the tag goes in the first FILE_TAG_LINES lines. +FILE_CHECKS = ('file_lines',) + tuple(HEADER_LIMITS) +CHECKS = (('file_lines', 'function_lines', 'nesting', 'parameters') + tuple(CLASS_LIMITS) + + tuple(HEADER_LIMITS) + STYLE_CHECKS) FILE_TAG_LINES = 20 MIN_JUSTIFICATION_CHARS = 40 MIN_JUSTIFICATION_WORDS = 6 @@ -67,9 +74,11 @@ def attach(path, source, allowances, metrics): allowed, errors = {}, [] for allowance in allowances: for check in allowance.checks: + if check in STYLE_CHECKS: + continue targets = [m for m in metrics if m.check == check and covers(allowance, m, lines)] if not targets: - where = (f'in the first {FILE_TAG_LINES} lines' if check == 'file_lines' + where = (f'in the first {FILE_TAG_LINES} lines' if check in FILE_CHECKS else 'on or directly above the function or class signature') errors.append(Finding(path, allowance.line, f'code-health tag: {check} attaches to ' f'nothing; put the tag {where}')) @@ -78,8 +87,30 @@ def attach(path, source, allowances, metrics): return allowed, errors +def waive(path, source, allowances, findings): + """(findings left, bypass notices, errors for style tags that attach to nothing). + + A STYLE_CHECKS tag goes on the finding's line or in the comment block directly above it.""" + lines = normalized_lines(source) + kept, notices, used = [], [], set() + for finding in findings: + tag = next((a for a in allowances if finding.check in a.checks + and covers(a, finding, lines)), None) + if tag is None: + kept.append(finding) + continue + used.add((tag.line, finding.check)) + notices.append(Finding(path, finding.line, f'{finding.check} bypassed by the tag at line ' + f'{tag.line}: "{tag.justification}"')) + errors = [Finding(path, a.line, f'code-health tag: {check} attaches to nothing; put the tag ' + 'on or directly above the flagged line') + for a in allowances for check in a.checks + if check in STYLE_CHECKS and (a.line, check) not in used] + return kept, notices, errors + + def covers(allowance, metric, lines): - if metric.check == 'file_lines': + if metric.check in FILE_CHECKS: return allowance.line <= FILE_TAG_LINES if allowance.line == metric.line: return True diff --git a/devtools/code_health/config.py b/devtools/code_health/config.py index bf12787dfd..7ea3902798 100644 --- a/devtools/code_health/config.py +++ b/devtools/code_health/config.py @@ -6,13 +6,19 @@ 'single_method_interface': 0, } +# Distinct public namespace-scope names (types, aliases, functions, constants) per header. +HEADER_LIMITS = {'header_symbols': 8} + # Advisory floors: new or changed values above these (and within the limit) are reported. # Changed files above their floor are always reported. -ADVISORY = {'file_lines': 500, 'function_lines': 25, 'public_methods': 5} +ADVISORY = {'file_lines': 500, 'function_lines': 25, 'public_methods': 5, + 'header_symbols': 6} HINTS = { 'public_methods': 'split the class or move helpers into free functions', 'public_data': 'make data private, or split it into a plain struct without methods', + 'header_symbols': 'split the header by concern, or move helpers into a detail namespace ' + 'or the .cpp', 'single_method_interface': 'prefer a callable (MoveOnlyFunction / std::function) ' 'over a single-method interface', } diff --git a/devtools/code_health/findings.py b/devtools/code_health/findings.py index d736a9c01f..b482ab8416 100644 --- a/devtools/code_health/findings.py +++ b/devtools/code_health/findings.py @@ -12,6 +12,7 @@ class Finding: path: str line: int message: str + check: str = '' # set on findings an allow tag can bypass (allow.STYLE_CHECKS) def __str__(self): return f'{self.path}:{self.line}: {self.message}' diff --git a/devtools/code_health/header_api.py b/devtools/code_health/header_api.py new file mode 100644 index 0000000000..f6f48cf41b --- /dev/null +++ b/devtools/code_health/header_api.py @@ -0,0 +1,149 @@ +"""Header API shape: prefix overloads and the number of public namespace-scope names. + +Both read the statements `style.py` lexes. An overload pair whose parameter types are a +strict prefix of each other is what a default argument or a `std::optional` parameter +replaces. The header symbol count is a Metric, compared with the base like function length. +""" +import re + +from .config import HEADER_LIMITS +from .findings import Finding +from .metrics import Metric +from .style import (HEADERS, added_lines, default_split, internal, parameter_type, rule, scopes, signature, + statements, strip_code, without_templates) + +SYMBOL = '
' +TYPE_HEAD = re.compile(r'(?:template\s*<.*>\s*)?(?:enum(?:\s+(?:class|struct))?|class|struct|union)\b(.*)$') +DECLARATION_KEYWORDS = re.compile(r'(using|typedef|namespace|template|class|struct|union|enum|friend' + r'|static_assert|return|extern\s*")\b') +VALUE = re.compile(r'(?:[\w:]+(?:<.*>)?[\s\*&]+)+?[\*&\s]*([A-Za-z_]\w*)\s*(?:\[[^\]]*\])?') + + +def public(enclosing): + """Namespace scope outside anonymous, `detail`, `internal` and `*Internal` namespaces.""" + for kind, name in enclosing: + if kind == 'linkage': + continue + if kind != 'namespace' or not name or internal(name.split('::')[-1]): + return False + return True + + +def normalized(text): + return ' '.join(text.replace('*', ' * ').replace('&', ' & ').split()) + + +# ---- Prefix overloads --------------------------------------------------------------- + +def overload_candidate(statement, enclosing): + """(scope, name, parameter types) of a declaration that may pair with a prefix overload.""" + sig = signature(statement) + if sig is None or '::' in sig.name or sig.name.startswith('~') or 'operator' in statement.text: + return None + if re.search(r'=\s*delete\b', sig.tail) or any('...' in p for p in sig.params): + return None + if enclosing and enclosing[-1][0] == 'class' and sig.name == enclosing[-1][1]: + return None # constructor + if not all(kind in ('namespace', 'linkage', 'class') for kind, _ in enclosing): + return None + params = tuple(normalized(parameter_type(p)) for p in sig.params if p.strip() not in ('', 'void')) + return enclosing, sig.name, params + + +def prefix_overloads(path, source, previous_source=None): + """Errors for same-scope overloads whose parameter types are a strict prefix of another's. + + At least one of the pair must be on a line added since `previous_source`.""" + if not path.endswith(HEADERS): + return [] + previous = normalize(previous_source).split('\n') if previous_source is not None else None + added = added_lines(normalize(source).split('\n'), previous) + found = [] + for statement, enclosing in scopes(statements(strip_code(normalize(source)))): + candidate = overload_candidate(statement, enclosing) + if candidate: + found.append((candidate, statement.line)) + errors, reported = [], set() + for (scope, name, short), line in found: + for (other_scope, other_name, long), other_line in found: + if (scope, name) != (other_scope, other_name) or len(short) >= len(long) \ + or long[:len(short)] != short: + continue + if line not in added and other_line not in added: + continue + at = max(line, other_line) + if (at, name) in reported: + continue + reported.add((at, name)) + errors.append(Finding(path, at, rule( + f'{name}: overload ({", ".join(short)}) is a prefix of ({", ".join(long)}) at line ' + f'{other_line if at == line else line}; use one function with a default argument ' + 'or a std::optional parameter', 'fork AGENTS.md "Functions"'), + check='overload_prefix')) + return sorted(errors, key=lambda f: f.line) + + +# ---- Public names per header -------------------------------------------------------- + +def type_name(text): + head = TYPE_HEAD.match(text) + if not head: + return None + rest = re.split(r'(?\s*)?using\s+([A-Za-z_]\w*)\s*=', text) + if alias: + return alias.group(1) + if text.startswith('typedef'): + names = re.findall(r'([A-Za-z_]\w*)\s*\)?\s*(?:\(.*\))?\s*(?:\[[^\]]*\])?$', text) + return names[-1] if names else None + if statement.end == '{': + named = type_name(text) + if named: + return named + elif TYPE_HEAD.match(text): + return None # forward or opaque declaration + sig = signature(statement) + if sig is not None: + if '::' in sig.name or 'operator' in text: + return None + return sig.name + head = re.sub(r'^template\s*<.*?>\s*', '', text) + head = default_split(head.rstrip('=').strip() if statement.end == '{' else head)[0] + if DECLARATION_KEYWORDS.match(head) or '(' in without_templates(head): + return None + value = VALUE.fullmatch(head) + return value.group(1) if value else None + + +def public_names(source): + """{name: first line} of distinct public namespace-scope names in a header.""" + names = {} + for statement, enclosing in scopes(statements(strip_code(normalize(source)))): + if not statement.namespace_scope or not public(enclosing): + continue + name = declared_name(statement) + if name and name not in names: + names[name] = statement.line + return names + + +def measure_header(path, source): + if not path.endswith(HEADERS): + return [] + count = len(public_names(source)) + return [Metric(path, SYMBOL, 'header_symbols', count, HEADER_LIMITS['header_symbols'], 1)] + + +def normalize(source): + return source.replace('\r\n', '\n').replace('\r', '\n') diff --git a/devtools/code_health/style.py b/devtools/code_health/style.py index c7f20b4b1c..a9ae45d04b 100644 --- a/devtools/code_health/style.py +++ b/devtools/code_health/style.py @@ -274,6 +274,22 @@ def rule(message, *cites): RETURN_MOVE = re.compile(r'\breturn\s+std::move\s*\(\s*([A-Za-z_]\w*)\s*\)\s*;') QT_INCLUDE = re.compile(r'^\s*#\s*include\s*]*$', before): + continue + found.append(match.start()) + return found def function_start(bodies, line): @@ -296,6 +312,11 @@ def line_checks(path, raw, code, added, statements_): for pattern, exempt, message, *cites in BANS: if pattern.search(line) and not path.startswith(exempt or ('\0',)): errors.append(Finding(path, number, rule(message, *cites))) + if char_pointers(line): + errors.append(Finding(path, number, rule( + 'no char* / char[] in new code; use std::string, or std::string_view ' + '(inline constexpr std::string_view for constants)', 'fork AGENTS.md "Strings"'), + check='char_pointer')) if BIND.search(line): errors.append(Finding(path, number, rule('use a lambda, not std::bind', 'TotW #108'))) if not allocator and (NEW.search(line) or DELETE.search(line)): diff --git a/devtools/code_health/tests/test_cli.py b/devtools/code_health/tests/test_cli.py index 1e0a77a10f..e415329321 100644 --- a/devtools/code_health/tests/test_cli.py +++ b/devtools/code_health/tests/test_cli.py @@ -66,6 +66,22 @@ def test_new_file_must_meet_limits(self): self.source(51, 'Common/new.hpp') self.assertEqual(1, self.cli().returncode) + def test_header_api_rules_and_style_waiver(self): + self.source(3) + self.commit() + names = ''.join(f'// Name {i}.\nvoid f{i}();\n' for i in range(9)) + (self.root / 'Common/api.h').write_text( + 'namespace A{\n' + names + '// Overload.\nvoid f0(int x);\n' + '// code-health: allow char_pointer -- the C library callback hands us this pointer\n' + 'void on_text(const char* text);\n} // namespace A\n') + self.git('add', '.') + result = self.cli() + self.assertEqual(1, result.returncode, result.stderr) + self.assertIn('header_symbols=10, limit=8', result.stderr) + self.assertIn('f0: overload () is a prefix of (int)', result.stderr) + self.assertNotIn('char*', result.stderr) + self.assertIn('Bypass: Common/api.h:23: char_pointer bypassed by the tag at line 22', result.stdout) + def test_unchanged_bad_function_in_changed_file_passes(self): self.source(60) self.commit() diff --git a/devtools/code_health/tests/test_header_api.py b/devtools/code_health/tests/test_header_api.py new file mode 100644 index 0000000000..eef0dba6a6 --- /dev/null +++ b/devtools/code_health/tests/test_header_api.py @@ -0,0 +1,196 @@ +import unittest + +from devtools.code_health import allow +from devtools.code_health.header_api import measure_header, prefix_overloads, public_names +from devtools.code_health.policy import advisories, evaluate +from devtools.code_health.style import check + + +CPP = 'SerialPrograms/Source/VideoGames/PokemonFRLG/Programs/Example.cpp' +HEADER = 'SerialPrograms/Source/VideoGames/PokemonFRLG/Programs/Example.h' +CHAR_MESSAGE = 'no char* / char[] in new code' + + +def wrap(body): + return 'namespace PokemonAutomation{\n' + body + '\n} // namespace PokemonAutomation\n' + + +def char_errors(source, path=CPP, previous=None): + return [f for f in check(path, source, previous)[0] if CHAR_MESSAGE in f.message] + + +class CharPointerTests(unittest.TestCase): + def test_flags_char_pointer_and_array_declarations(self): + for line in ('const char* name();', + 'void f(const char* text);', + 'void f(char const* text);', + 'void f(char** list);', + 'char* buffer = nullptr;', + 'inline constexpr const char* SLUG = "x";', + 'struct A{ const char* m_name; };', + 'char name[16];', + 'const char label[] = "x";', + 'std::function on_text;'): + with self.subTest(line=line): + found = char_errors(wrap(line)) + self.assertEqual(1, len(found), line) + self.assertEqual('char_pointer', found[0].check) + + def test_ignores_other_char_types_literals_comments_and_main(self): + for line in ('unsigned char* bytes;', 'signed char* bytes;', 'const unsigned char table[4] = {};', + 'char8_t* a; char16_t* b; char32_t* c; wchar_t* d;', 'uint8_t* bytes;', + 'char c = \'x\';', 'void f(char c);', 'std::string s = "const char* x";', + '// returns const char* today', 'int main(int argc, char** argv);', + 'auto p = reinterpret_cast(data);', 'std::vector bytes;'): + with self.subTest(line=line): + self.assertEqual([], char_errors(wrap(line))) + + def test_only_added_lines(self): + old = wrap('const char* legacy();') + self.assertEqual([], char_errors(wrap('const char* legacy();\nint x();'), previous=old)) + self.assertEqual(1, len(char_errors(wrap('const char* legacy();\nconst char* fresh();'), + previous=old))) + + +class PrefixOverloadTests(unittest.TestCase): + def messages(self, source, previous=None, path=HEADER): + return [f.message for f in prefix_overloads(path, source, previous)] + + def test_flags_strict_prefix_overloads(self): + source = wrap('std::string checkpoint_start_line(std::string_view checkpoint);\n' + 'std::string checkpoint_start_line(std::string_view checkpoint, size_t events);') + found = prefix_overloads(HEADER, source) + self.assertEqual(1, len(found)) + self.assertEqual(3, found[0].line) + self.assertEqual('overload_prefix', found[0].check) + self.assertIn('checkpoint_start_line: overload (std::string_view) is a prefix of ' + '(std::string_view, size_t)', found[0].message) + for body in ('void f();\nvoid f(int x);', + 'struct A{\n void run(int a) const;\n void run(int a, bool b) const;\n};', + 'inline void f(const Foo& a){\n}\nvoid f(const Foo& b, int c = 0);'): + with self.subTest(body=body): + self.assertEqual(1, len(self.messages(wrap(body))), body) + + def test_ignores_different_types_ctors_operators_and_other_scopes(self): + for body in ('std::string to_string(A a);\nstd::string to_string(B b);', + 'void f(int a);\nvoid f(std::string a, int b);', + 'struct A{\n A();\n A(int x);\n};', + 'struct A{\n A& operator=(const A&);\n A& operator=(const A&, int);\n};', + 'void f(int a);\nstruct A{\n void f(int a, int b);\n};', + 'namespace x{\nvoid f(int a);\n} // namespace x\nvoid f(int a, int b);', + 'void f(int a);\nvoid f(int a);', + 'void f(int a);\nvoid f(int a, int b) = delete;', + 'template void log(T v);\ntemplate ' + 'void log(T v, R... rest);'): + with self.subTest(body=body): + self.assertEqual([], self.messages(wrap(body)), body) + self.assertEqual([], self.messages(wrap('void f();\nvoid f(int x);'), path=CPP)) + + def test_needs_one_of_the_pair_on_an_added_line(self): + old = wrap('void f();\nvoid f(int x);') + self.assertEqual([], self.messages(wrap('void f();\nvoid f(int x);\nvoid g();'), old)) + self.assertEqual(1, len(self.messages(wrap('void f();\nvoid f(int x, int y);'), old))) + + +class HeaderSymbolTests(unittest.TestCase): + STORY = wrap('''class StoryCheckpoint{ +public: + void run(); + void stop(); +}; +struct UnsupportedStep{ std::string reason; }; +std::string unsupported_step(std::string_view reason); +std::string story_event(std::string_view name); +StoryCheckpoint build_story_checkpoint(int index); +StoryCheckpoint build_story_checkpoint(int index, int events); +using PreparedSteps = std::vector; +void lint_story_checkpoint(const StoryCheckpoint& checkpoint); +void record_story_stats(const StoryCheckpoint& checkpoint); +enum class Facing{ UP, DOWN }; +Facing facing_of(int direction);''') + + def test_counts_distinct_public_names(self): + self.assertEqual(['StoryCheckpoint', 'UnsupportedStep', 'unsupported_step', 'story_event', + 'build_story_checkpoint', 'PreparedSteps', 'lint_story_checkpoint', + 'record_story_stats', 'Facing', 'facing_of'], + list(public_names(self.STORY))) + + def test_counts_constants_and_typedefs_not_internals(self): + source = wrap('''inline constexpr std::string_view SLUG = "x"; +extern const int LIMIT; +const Point ORIGIN{0, 0}; +typedef int Handle; +template inline constexpr bool is_step = false; +template <> struct std::hash{ size_t operator()(const Point&) const; }; +class Forward; +#define MACRO_NAME 1 +std::ostream& operator<<(std::ostream& out, const Point& p); +void Point::method(){ +} +namespace detail{ +int helper(); +} // namespace detail +namespace{ +int file_helper(); +} // namespace +namespace StoryInternal{ +int other_helper(); +} // namespace StoryInternal +struct Holder{ + int member; + static int count(); +};''') + self.assertEqual(['SLUG', 'LIMIT', 'ORIGIN', 'Handle', 'is_step', 'Holder'], + list(public_names(source))) + + def test_nested_public_namespace_counts(self): + self.assertEqual(['f', 'g'], list(public_names(wrap('void f();\nnamespace Story{\nvoid g();\n' + '} // namespace Story')))) + + def test_limit_and_base_comparison(self): + nine = measure_header(HEADER, self.STORY) + self.assertEqual(10, nine[0].value) + self.assertEqual(1, len(evaluate(nine, []))) + self.assertIn('header_symbols=10, limit=8', evaluate(nine, [])[0].message) + self.assertEqual([], evaluate(nine, nine)) # an unchanged legacy count may stay + smaller = measure_header(HEADER, wrap('void a();\nvoid b();\nvoid c();\nvoid d();\nvoid e();' + '\nvoid f();\nvoid g();')) + self.assertEqual(7, smaller[0].value) + self.assertEqual([], evaluate(smaller, [])) + self.assertEqual(1, len(advisories(smaller, []))) + self.assertEqual([], measure_header(CPP, self.STORY)) + + def test_file_level_tag_attaches(self): + source = ('// code-health: allow header_symbols -- one table of story names that callers ' + 'always use together\n' + self.STORY) + metrics = measure_header(HEADER, source) + allowances, errors = allow.parse(HEADER, source) + self.assertEqual([], errors) + allowed, misplaced = allow.attach(HEADER, source, allowances, metrics) + self.assertEqual([], misplaced) + self.assertEqual([], evaluate(metrics, [], allowed)) + + +class StyleWaiverTests(unittest.TestCase): + def test_tag_waives_one_style_finding(self): + source = wrap('// code-health: allow char_pointer -- the C library callback hands us this ' + 'pointer and owns it\nvoid on_text(const char* text);\nconst char* other();') + allowances, errors = allow.parse(CPP, source) + self.assertEqual([], errors) + kept, notices, unattached = allow.waive(CPP, source, allowances, char_errors(source)) + self.assertEqual([4], [f.line for f in kept]) + self.assertEqual(1, len(notices)) + self.assertEqual([], unattached) + + def test_unattached_style_tag_fails(self): + source = wrap('// code-health: allow overload_prefix -- the overload set is fixed by Qt ' + 'signal signatures here\nvoid f();') + allowances, _ = allow.parse(HEADER, source) + kept, notices, unattached = allow.waive(HEADER, source, allowances, []) + self.assertEqual(([], []), (kept, notices)) + self.assertEqual(1, len(unattached)) + self.assertEqual([], allow.attach(HEADER, source, allowances, [])[1]) + + +if __name__ == '__main__': + unittest.main() diff --git a/devtools/code_health/tests/test_style.py b/devtools/code_health/tests/test_style.py index ba8b7b487c..6aa65d344e 100644 --- a/devtools/code_health/tests/test_style.py +++ b/devtools/code_health/tests/test_style.py @@ -158,7 +158,7 @@ def test_primary_template_before_its_specialization_is_not_a_forward_declaration def test_top_level_const_by_value_parameters_in_header_declarations(self): self.assertFlags("'const int count'", wrap('void f(const int count, const std::string& name);'), HEADER) self.assertFlags("'const size_t index = 0'", wrap('struct A{\n A(const size_t index = 0);\n};'), HEADER) - self.assertClean(wrap('void f(const std::string& a, const char* b, const std::vector c_ref,\n' + self.assertClean(wrap('void f(const std::string& a, const int* b, const std::vector c_ref,\n' ' int d, std::function&& e);')) self.assertEqual([], self.errors(wrap('inline void f(const int x){\n g(x);\n}'), HEADER)) self.assertEqual([], self.errors(wrap('void f(const int count);'))) # .cpp declaration