topic5: docs

[SCOFRD]

No description provided.

[github-actions]

🤖 AI Code Review

共审查 8 个变更文件

📁 .github/workflows/ci.yml

🟡 **Missing `--junit-xml`** — New "Run assembly-beautifier regressions" step (line ~87) lacks `--junit-xml=benchmark_reports/test_results.xml`. Failures won't surface in GitHub's Checks UI annotations, unlike every other test step above it.

🟡 **Benchmark failure blocks CI** — `benchmarks.bench_asm_beautifier` (line ~228) is not wrapped in `|| true` or a conditional. A timing flake on a shared runner or a non-critical regression in the beautifier will turn a green build red. If this benchmark is informational, consider:
    ```yaml
    continue-on-error: true
    ```

🟡 **`--repeats 20` is heavy for CI** — 20 iterations of a full-file beautify benchmark on a constrained runner can add significant wall time and is more susceptible to noise (shared CPU, GC pauses). Consider reducing to ~5 with a note, or gating behind a nightly/manual trigger.

🟡 **No guard on intermediate file** — The beautifier benchmark reads `benchmark_reports/cnn_scratchv.s` produced by the prior step. If the CNN generation step fails (e.g., ONNX download fails and the `if` guard exits early), this step dies with an opaque "file not found." Add a precondition check:
    ```bash
    test -f benchmark_reports/cnn_scratchv.s || { echo "::warning::skipping"; exit 0; }
    ```

💭 **`mkdir -p benchmark_reports`** — Several steps write into `benchmark_reports/` but I don't see an explicit `mkdir -p` for it in this diff. If it's created earlier in the workflow, fine — but a defensive `mkdir -p` at the top of each job that writes there prevents confusing failures on fresh checkout / cache miss.

💭 **Minor inconsistency in Summary append** — Lines ~349–352: the `echo ""` separator appears *before* the `asm_beautifier_summary` block but *after* the `github_summary` block. The surrounding code uses a uniform pattern (`if` + `cat` + blank line). Consider matching the existing style for readability.

---

📁 docs/topic5汇编代码美化器开发文档.md

🔴 **Dangling cross-reference** — DoD checklist: "函数入口识别与标题插入位置与设计文档第 2.4 节一致"
   Section 2.4 does not exist in this document. Either add it or correct the reference. The sibling bullet correctly references §2.2 which exists — this looks like a stale section number.

🔴 **`parse_asm_line` default `lineno=0` contradicts spec** — The prose says lineno is "从 1 开始", but the signature defaults to 0. A caller invoking `parse_asm_line("add a0, a1, a2")` without lineno gets an invalid line number. Default to `1` or make the parameter required.

🟡 **Label padding width has no floor, unlike opcode/operands** — §2.2: `min(max_label, 30)` vs `min(max(max_opcode, 8), 12)`. If all labels are ≤3 chars the column is only 3 wide, causing visual misalignment with opcode. If intentional, add a note explaining the asymmetry; otherwise set a floor (e.g. `min(max(max_label, 8), 30)`).

🟡 **BNF `comment` rule omits string-literal context** — The grammar says `comment ::= "#" text` unconditionally, but the prose correctly states `#` inside string literals does not start a comment. The BNF can't express "outside string literals" — add a prose note adjacent to the grammar block pointing to the string-aware scanning rule.

🟡 **DoD references "四类段标题映射" but the document never enumerates them** — The four section types and the `60 × '='` separator format are only described by cross-reference to an external doc. Include at minimum the four section types (e.g. `.text`, `.data`, `.rodata`, `.bss`) inline or in a footnote so this document is self-contained.

🟡 **CLI missing `--encoding`** — `beautify_file()` exposes `encoding`, but no CLI flag for it. If non-UTF-8 input must be supported, add `--encoding` to the CLI table. If it's intentionally Python-API-only, add a note.

💭 **Progress table shows coding/testing complete but no code links** — Add links to the actual implementation files or PR URL so reviewers can cross-reference the doc against code in one click.

---

📁 tests/test_asm_beautifier.py

🔴 Blocker: Complete test deletion without replacement — 161 lines of tests removed, zero added. This removes coverage for _parse_line, _gen_comment, beautify_asm, and CLI for asm_beautifier.

🟡 Question: Is scratchv.backend.asm_beautifier also being deleted? — If yes, this is fine but the commit message should make that clear. If no, you just lost all test coverage for a live module.

🟡 Notable lost coverage — The deleted tests covered meaningful edge cases:

• test_parse_label_only / test_parse_comment_only — guard against parse ambiguity
• test_preserves_original_comment — verifies user comments aren't clobbered
• test_section_header_inserted — validates .text/.data boundary handling
• test_nop_comment / test_unknown_opcode — fallback behavior

These aren't trivial checks. If you're restructuring, these should be ported, not dropped.

💭 If this deletion is intentional (module removed, refactored elsewhere), add a brief comment in the commit message or PR description explaining why, so future reviewers don't wonder about the lost coverage.

---

📁 tests/test_asm_beautifier_blackbox.py

🔴 CLI_SCRIPT may point to wrong/nonexistent file — Lines 12-13: The fallback Path(__file__).with_name("asm_beautifier.py") looks in tests/, not the project root. If neither path matches, tests will all fail with an unhelpful FileNotFoundError. Consider a single explicit path or a clear assertion/pytest.skip if not found.

🟡 TimeoutExpired not handled — Line 24: subprocess.run(timeout=10) raises subprocess.TimeoutExpired on hang. This will surface as a raw exception, not a clean test failure. Wrap in try/except and pytest.fail("timed out").

🟡 Inconsistent stderr assertions across error tests — Line 87: test_cli_invalid_utf8_returns_one doesn't assert "Traceback" not in completed.stderr, unlike test_cli_missing_input_file_returns_one (line 76). Both should consistently verify no traceback leaks.

🟡 test_cli_unwritable_output_path_returns_one name is misleading — Line 89: Uses a missing parent directory, not an unwritable file. The test name overstates the scenario. Either rename to ..._missing_parent_dir_... or actually test chmod 000 on a directory.

💭 No test for --help — Adding a simple run_cli("--help") → exit 0, "usage:" in stdout/stderr would lock in CLI help behavior.

💭 Type annotation on run_cli — Line 15: *args: str | Path is fine for the call sites, but subprocess.CompletedProcess[str] return type annotation requires Python 3.9+ — consistent with from __future__ import annotations at line 3, so this is fine.

---

📁 tests/test_asm_beautifier_comments.py

Code Review: tests/test_asm_beautifier_comments.py

🟡 Dead key in _SAMPLE_OPERANDS — The "rs" key is never used by any parametrized test or the smoke test. Only rs1/rs2 are referenced. Remove it or add a test case that exercises it.

🟡 test_all_parser_instructions_have_working_templates is a weak smoke test — Only checks assert comment (non-empty). A template that produces garbage like "{} = {} + {}" (unformatted placeholders) or a typo would still pass. Consider at minimum:

assert "{}" not in comment, f"{opcode} left unfilled placeholders"

💭 No test for _gen_comment receiving more operands than expected — If the implementation silently ignores extras (e.g., _gen_comment("add", ["a0","a1","a2","a3"])), that's fine, but it's untested behavior. Worth a test case to lock it down or document the contract.

💭 Private symbol coupling — _INST_COMMENTS and _gen_comment are underscore-prefixed internals. Acceptable for unit tests, but if these are ever refactored, tests will break even when behavior is unchanged. If the public beautify_asm covers enough surface, consider reducing direct imports of internals.

Summary: Test coverage is strong — good mix of parametrized correctness cases, completeness smoke test, edge cases (implicit ra, nop, directives, align=False), and ABI aliasing tests. The test file is clean and well-organized. The two suggestions above are the only substantive improvements.

---

📁 tests/test_asm_beautifier_formatting.py

Code Review: tests/test_asm_beautifier_formatting.py

💭 **Magic numbers for width caps** — `test_padding_caps_do_not_truncate_long_fields`:
   Hardcodes `label=30` and `operands=40`. `test_short_fields_use_minimum_widths`
   already references `OPCODE_WIDTH_MIN` / `OPERANDS_WIDTH_MIN` constants — if
   analogous `*_WIDTH_MAX` constants exist, import them for consistency.

🟡 **`add_comments=True` is untested** — Every test uses `add_comments=False`.
   If `add_comments=True` changes alignment/column behavior (e.g., reserves space
   for added comments), that path has zero coverage. Add at least one case.

💭 **Incomplete-operand warning is an implicit contract** — `test_two_pass_alignment...`
   asserts the exact string `# [warning: operand missing]`. This is a user-visible
   format string; a future wording tweak silently breaks the test. Consider
   matching with a pattern (`endswith("missing]")`) or documenting the contract
   in the docstring.

💭 **No trailing-newline-absent case** — `test_trailing_newline_is_preserved_when_aligning`
   covers the `\n` suffix. Consider also asserting behavior when input has no
   trailing newline, to pin down whether one is added or not.

---

📁 tests/test_asm_beautifier_integration.py

🔴 Bug: temp-file cleanup assertion is implementation-coupled — Line 552: list(tmp_path.glob(".output.s.*.tmp")) == [] hard-codes the temp-file naming pattern. If the implementation switches to tempfile.NamedTemporaryFile (random names), this test fails even though cleanup still works.
Suggestion: Assert no *.tmp files remain at all, e.g. assert not list(tmp_path.glob("*.tmp")).

🔴 Bug: monkeypatch on shared module object leaks scope — Line 539: monkeypatch.setattr(asm_beautifier.os, "replace", ...) patches the global os module (the attribute is a module reference, not a copy). monkeypatch will revert after the test, but during the test any other code touching os.replace in the same process is affected.
Suggestion: Prefer patching the call site directly if exposed, or accept this as an accepted trade-off and add a comment noting the global effect.

🟡 Suggestion: brittle alignment math in test_unknown_and_malformed_lines_keep_raw_code_and_add_warning — Lines 54-58 embed the alignment formula (max(_standard_comment_column(), len(source) + 2)). Any tweak to padding logic forces a test edit even if behavior is correct.
Suggestion: Assert relative invariants instead (e.g. result.index("#") >= len(source) + 1 and warnings are right-aligned across the parametrized cases), or factor the formula into a shared helper next to _standard_comment_column.

🟡 Suggestion: ABI-register-name test relies on a subtle mapping — Lines 526-536: assert "# ra = sp + gp" in result only holds because RISC-V x1/x2/x3 → ra/sp/gp. A future change to the naming table or to abi_register_names semantics will produce a cryptic failure.
Suggestion: Add a one-line comment explaining the mapping, or assert the comment contains the three ABI names independently.

🟡 Suggestion: missing negative/idempotency coverage for beautify_file — File-level tests only cover happy path and replace failure.
Suggestion: Add at least: (a) input_path == output_path behavior, (b) output directory missing / read error on input, (c) idempotency across beautify_file round-trips.

🟡 Suggestion: no edge-case input coverage — Empty string, single-line input, Windows CRLF line endings, UTF-8 BOM, and trailing newline handling are all untested. These are common real-world cases for an asm beautifier.
Suggestion: Add a parametrized test covering these inputs and document expected normalization.

💭 Nit: mixed split("\n") vs splitlines() — Lines 215, 340 use split("\n") to preserve whitespace-only lines, while other tests use splitlines(). The difference is intentional but unexplained.
Suggestion: One-line comment where the distinction matters.

💭 Nit: _standard_comment_column is dead code outside tests that call it — Only 2 call sites. Fine for now; if more tests need it, keep it; otherwise inline it.

💭 Nit: test_generated_warning_is_not_duplicated could parametrize over all warning types — Currently covers 3 cases but not all warning kinds emitted by the beautifier.
Suggestion: Add the remaining warning categories to the parametrize list to fully cover idempotency.

💭 Nit: missing add_comments=True coverage — Nearly every test pins add_comments=False to isolate the warning logic. There's no integration test for the default add_comments=True path beyond test_default_options_merge_comments_in_structured_program and test_complete_program_receives_structure_comments_and_alignment.
Suggestion: Add a focused test that a generated comment is never appended when add_comments=False (currently implicit), to lock that contract.

---

📁 tests/test_parser_for_beautifier.py

🟡 **Weak assertion in `test_representative_extended_instruction_is_valid`** — Lines 109-113:
Only asserts `parse_status == "valid"`, missing `opcode` check. If `parse_asm_line`
silently returns a wrong opcode with status "valid", this test passes. Compare with
other parameterized tests that assert `opcode`.

🟡 **`field_lengths` not tested for non-"valid" statuses** — `TestValidFieldLengths` only
covers `parse_status == "valid"`. If downstream code reads `field_lengths` for
`metadata_label`, `incomplete_operands`, `unknown_opcode`, or `malformed` lines,
missing coverage here could hide bugs. Consider adding parametrized cases for each status.

🟡 **No tests for leading whitespace/tabs** — E.g., `"  add x1, x2, x3"` or
`"\tmain: addi sp, sp, -16"`. Real assembler output frequently has indentation;
behavior is untested.

🟡 **`test_parenthesized_comma_remains_in_one_operand`** — Line 146:
`lw a0, 8(sp, t0)` is invalid RISC-V syntax. Add a comment noting this is testing
paren-matching logic, not instruction validity, to avoid confusing future readers.

🟡 **Missing edge cases for `parse_asm`** — No test for empty input `parse_asm("")`
or input without trailing newline `parse_asm("ret")`. The existing test (line 441)
covers 1-based numbering but not boundary conditions.

🟡 **`test_hash_inside_unclosed_memory_operand_starts_comment`** — Lines 417-422:
The test exercises both `#`-inside-paren comment splitting AND unclosed-paren
malformed detection, but the name only mentions the comment aspect. The assertion
that `operands_str == "a0, 0(sp"` (truncated at `#`) is the critical behavior —
consider splitting into two focused tests or renaming to reflect both behaviors.

💭 **Implicit string concatenation in `test_nop_with_operator_comment_preserves_original_comment`** —
Lines 356-358: Using Python's implicit concatenation for a multi-line expected comment
makes it hard to see that `\n` is embedded. A single string with explicit `"\n"` is
clearer.

💭 **`test_directive_allows_top_level_spaces`** — Lines 52-72: The name implies
testing leading/trailing whitespace handling, but the cases only test spaces *between*
the directive and its arguments. Rename to something like `test_directive_preserves_internal_spaces_in_operands`.

---

[SCOFRD]

test_asm_beautifier.py的测试已迁移，test_asm_beautifier_comments.py、test_asm_beautifier_formatting.py、test_parser_for_beautifier.py负责单元测试，test_asm_beautifier_integration.py负责集成测试，test_asm_beautifier_blackbox.py负责黑盒测试,bench_asm_beautifier.py负责压力测试