Skip to content

topic5: docs - #36

Open
SCOFRD wants to merge 10 commits into
ScratchV-Compiler:mainfrom
SCOFRD:feature/asm_beautifier
Open

topic5: docs#36
SCOFRD wants to merge 10 commits into
ScratchV-Compiler:mainfrom
SCOFRD:feature/asm_beautifier

Conversation

@SCOFRD

@SCOFRD SCOFRD commented Jul 31, 2026

Copy link
Copy Markdown

No description provided.

@github-actions

github-actions Bot commented Jul 31, 2026

Copy link
Copy Markdown

🤖 AI Code Review

共审查 10 个变更文件
⚠️ 另有 1 个文件超过上限(最多 10 个)未审查

📁 benchmarks/bench_asm_beautifier.py

🔴 阻塞:生成的汇编可能包含重复标签_gen_random_asmif index % 15 == 0index=0index=15 时都会添加 label_0:,导致重复定义。汇编器会报错,使 benchmark 无法运行。建议使用唯一标签,例如 label_{index // 15} 或维护计数器。

🟡 建议:synthetic_1k 未在 diff 中显示定义run_all_benchmarksbenchmarks 字典引用了 synthetic_1k,但 diff 未显示其定义。若未定义则运行时会 KeyError,请确认该键已存在。

🟡 建议:feature impact 未覆盖 abi_register_namesbench_beautify 新增了该参数,但 run_all_benchmarks 的 feature impact 部分仅测试 aligncomments,建议增加该维度的测试以完整评估性能影响。

💭 微瑕:字符串前导空行变化_SIMPLE_ASM 等从 """\n.text 改为 """.text,去掉了首行空行。这不影响功能,但若美化器对空行敏感,可能轻微改变输出,建议确认预期行为。

💭 微瑕:_gen_random_asmret 可能出现在非末尾 — 虽然合法,但可能使汇编语义不自然,建议将 ret 仅作为可选结尾,或将其从随机操作中移除(除非意图测试异常路径)。

其余变更(类型注解、确定性检查、参数校验)合理,无问题。


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

🔴 引用错误:验收标准中引用了不存在的章节
验收标准要求“段标题映射、标题插入顺序和 60 个 = 分隔线与设计文档第 2.2 节一致”和“函数入口识别与标题插入位置与设计文档第 2.4 节一致”,但文档中仅存在 2.1、2.2、2.3 节,第 2.4 节不存在,且 2.2 节(内部设计)并未以独立子节(如 2.2.1、2.2.2)明确段标题和函数识别的具体规则。这会导致开发者无法准确定位实现细节,影响验收一致性。
建议:修正引用,将段标题映射和函数识别规则补充到 2.2 节中(可新增子节 2.2.3 或 2.2.4),或直接引用“2.2 关键算法/流程”和“2.2 状态管理”等明确段落,并确保文档内标签一致。

🟡 内部设计缺少结构化细节
第 2.2 节“关键算法/流程”仅以文本列表描述步骤,未列出段标题映射表(如 .text# .text SECTION.data# .data SECTION 等)和函数标题的格式(# --- Function: symbol --- 及分隔线长度)。开发者需自行推断,可能引入偏差。
建议:在 2.2 节中增加一个表格,明确段标题、函数标题的格式、插入条件(如仅当 .text 段内非 metadata_label 的标签)和分隔线长度(60 个 =),并注明标题插入顺序。

💭 文档结构小节编号缺失
第 2.2 节“内部设计”下包含“数据结构变更”、“关键算法/流程”、“状态管理”三个独立段落,但未使用子节编号(如 2.2.1、2.2.2、2.2.3),影响快速定位。
建议:添加子节编号,并在验收标准中引用具体编号,提高可读性。

💭 BNF 注释风格不一致
top_level_comma ::= "," (* only when parenthesis depth is zero *) 将注释放在规则内,与 BNF 标准用法((* ... *) 作为独立注释)一致,但紧跟在 , 后可能引起误解。
建议:将注释单独成行,例如:

top_level_comma ::= ","
(* only when parenthesis depth is zero *)

📁 docs/topic5汇编代码美化器设计文档.md

🔴 术语不一致 — 第 2.5 节:“自动注释只输出对应的警告信息”与第 2.7 节矛盾。警告注释([warning: …])属于诊断信息,不受 add_comments 影响,应统一称为“警告注释”而非“自动注释”。

🟡 nop 特殊处理规则未在约束中统一声明 — 第 4.5 节规定 nop 带原始注释时不追加 no operation,但第 2.5 节只说“原始注释保留;若同时生成自动注释以 | 分隔”,未注明 nop 例外。建议在 2.5 节或 2.2 节明确此例外。

🟡 函数标题前空行规则不完整 — 第 2.4 节:“已有空行时不得重复追加”,但未规定若已有多个空行是否应缩减。建议明确“不修改已有空行数量,仅在无空行时插入一个空行”。

🟡 数据定义行是否属于 DIRECTIVES 集合未明确定义 — 第 4.4 节引用 DIRECTIVES 集合,但全文未列出该集合。建议在 2.2 节或附录中给出 DIRECTIVES 的完整列表,包括 .word.asciz 等数据定义指令。

💭 parse_status 优先级次序在文档中位置分散 — 第 2.6 节给出了判定优先级,但第 4.3 节也重复描述,且未引用 2.6 节。建议集中定义,避免两处一说导致后续维护漂移。

💭 示例输出与规范不完全匹配 — 5.1 节示例中 addi sp, sp, -32 的注释为 # sp = sp + -32,但规范 2.2 节要求操作数 sp,sp,-32 应规范化为 sp, sp, -32(逗号后空格)。示例中 addi sp, sp, -32 有两个空格,实际输出应为 addi sp, sp, -32 还是 addi sp, sp, -32?建议统一示例格式。

💭 --no-align 下的函数标签拆分未明确 — 第 2.4 节:“开启 --no-align 时仍插入函数标题,但函数标签及其同行指令保持原始布局,不强制拆分”。但若原始布局中标签与指令同行(如 main: addi sp, sp, -32),该行是否仍保持?文档未说明是否应保留原始行内顺序,建议补充。


📁 scratchv/backend/asm_beautifier.py

🔴 Breaking Change: Label placement — 对齐模式下标签始终独占一行,旧版本标签与指令同行。如果其他工具(如 diff、脚本)依赖旧格式,会破坏输出。建议在文档或 changelog 明确说明,或提供兼容选项。

🔴 Import error when running as scriptif __package__: 分支在直接执行 python asm_beautifier.py__package__None,回退到 from asm_parser_for_beautifier import ...,但该模块可能不在 sys.path 中,导致 ImportError。建议使用更健壮的导入方式,如 sys.path 调整或 __package__ 的动态设置。

🟡 fence instruction comment assumes default predicates — 在 _gen_comment 中,fence 的上下文默认使用 "iorw",但 RISC-V 规范要求显示指定前驱和后继,没有默认值。生成注释 memory barrier (iorw -> iorw) 可能误导用户。建议当操作数不足时,回退到更安全的描述,或直接省略注释。

🟡 Function label detection may miss some functions_collect_function_labels 使用 text_labels & (explicit_functions | global_symbols)call_targets & text_labels 来识别函数。但以下几种情况会被遗漏:

  • 通过 la 指令取地址调用的函数(常见于 C 库)
  • 通过 j 而非 call 跳转的非 .globl 函数
  • .type 值为 @function 但缺少 .globl 且未被 call 直接调用的局部函数(但 .type 已在 explicit_functions 中,因此会被 text_labels & explicit_functions 捕获,所以仅有此情况不漏)。但 text_labels 仅包含 .text 段中的标签,如果一个函数在 .globl 声明但位于 .text 段,也会被包含。总体风险较低,但值得注意。

🟡 Missing test coverage — 新引入的 scan_column_widths, _gen_comment, _format_parsed_line, _collect_function_labels 等核心函数没有可见的单元测试(diff 未包含测试文件)。建议为这些函数添加测试,特别是边界情况(如超长标签、无操作数指令、多行标签等)。

💭 Comment separator changed — 旧代码使用 " | " 分隔用户注释和自动注释,新代码使用 " | "(单空格)。虽然微调,但可能影响审美一致性,建议与旧版本保持一致或明确记录。

💭 Operand width calculation uses normalized stringscan_column_widthsmax_operands 使用 len(_format_operands(line)),而旧代码使用原始 operands_str 长度。_format_operands 会规范化操作数间的空格(如 a0, a1 变为 a0, a1),导致对齐宽度可能变窄,但结果更一致。这是有意行为,但可能轻微改变输出格式,建议在文档中注明。

💭 Atomic write does not preserve file permissions_write_text_atomic 使用 os.replace 替换文件,但临时文件的权限可能不同于原文件(如果目标已存在)。在共享环境中可能导致权限问题,可考虑在替换后复制权限位(如 shutil.copymode)。


📁 scratchv/backend/asm_parser_for_beautifier.py

🔴 缺少测试:该文件没有任何单元测试。应添加测试覆盖正常解析、异常情况(不完整操作数、未知 opcode、malformed 括号、注释等),以确保正确性。

🟡 重复的字符串解析逻辑_split_comment_split_operands 都实现了相似的字符遍历逻辑(跟踪引号、括号深度)。考虑提取公共辅助函数,减少重复。

🟡 未使用的参数_status_for 接受 label 参数但未使用。虽然注释解释,但建议移除或用 _ 占位,避免混淆。

💭 非标准指令INSTRUCTION_SPECS 中包含 max 指令,但 RISC-V 标准指令集没有 max(只有 max 是矢量扩展或伪指令?)。请确认是否属预期,或改名为 max 伪指令。

💭 括号不匹配的注释处理_split_comment 在遇到括号不匹配时立即返回 malformed,即使括号出现在注释之前且注释中包含闭合括号也可能被误判。但实际场景中很少见,可接受。


📁 tests/test_asm_beautifier.py

🔴 Blocker: Test file deleted without replacement — This diff removes the entire tests/test_asm_beautifier.py (161 lines), but no corresponding addition or migration is shown. This will eliminate all unit tests for _parse_line, _gen_comment, beautify_asm, and CLI importability. Unless these tests are moved to another file in the same commit, coverage will drop and CI will likely break.

🟡 Suggestion: Verify test migration — If the tests are being relocated to a different file (e.g., test_asm.py), ensure the diff includes that addition. If they are intentionally removed, provide a commit message explaining why (e.g., replaced by integration tests, feature removed). Otherwise, consider keeping the file or deprecating it gradually.


📁 tests/test_asm_beautifier_blackbox.py

🟡 脆弱断言 — 多个测试硬编码了精确的格式化输出(如 "add a0, a1, a2"),任何格式微调(如空格数量)都会导致测试失败。建议对关键行为(如存在注释、对齐是否生效)做更宽松的断言,或使用正则。

🟡 重复代码input_path.write_text(...) 在多个测试中重复,可提取为辅助函数(如 write_input)以减少冗余。

🟡 缺失组合测试 — 未覆盖 --no-comments--no-align 同时使用的场景,也未有参数组合的测试(如 --output + --no-comments),建议增加。

💭 stdin 未覆盖 — 若 CLI 支持从 stdin 读取,则该路径未测试;若不支持,建议在文档中明确。

💭 -o--output 等价性 — 测试中分别使用了两种形式,但未验证二者行为一致,可考虑只测试一种或显式验证等价。

以上均为可选改进,当前测试已覆盖主要流程和错误处理,质量良好。


📁 tests/test_asm_beautifier_comments.py

🟡 建议:测试存在 KeyError 风险test_all_parser_instructions_have_working_templates 中,_SAMPLE_OPERANDS 未覆盖 INSTRUCTION_SPECS 中所有可能的操作数角色(如 symbolpredecessor 等虽已提供,但若未来新增其他角色会直接 KeyError)。建议使用 .get() 提供默认值,或添加一个针对角色覆盖的测试。

💭 细节:依赖私有 API — 测试直接导入 _INST_COMMENTS_gen_comment,这些是内部实现细节,重构时可能破坏测试。若允许,可考虑通过 beautify_asm 的公共接口间接验证。

💭 格式绑定过紧test_no_align_only_appends_comment_without_reformatting_code 断言精确对齐 "add a0,a1,a2 # a0 = a1 + a2"(两个空格),若对齐逻辑微调则测试失败。虽为单元测试可接受,但依赖精确格式可能脆弱。

🔴 缺失关键场景测试 — 未测试 beautify_asm 对多行指令、非法操作数、以及 add_comments=True 时指令本身无注释但用户有注释的情况(如 nop 已有测试,但其他指令如 add 若有用户注释是否合并?已有 test_original_and_automatic_comments_are_merged 覆盖)。整体覆盖尚可,但可补充 luiauipc 等伪指令的注释验证。

💭 测试命名test_abi_aliases_do_not_change_non_register_rolesli x1,x2 的注释为 # ra = x2,但 x2 被当作立即数(非寄存器),此命名合理,但可更明确。

🟡 建议:使用 pytest.approx 或灵活匹配 — 若注释格式可能变化,可考虑使用正则或子串匹配,而非精确字符串,以提高测试弹性。例如 assert "a0 = a1 + a2" in result

总体测试质量高,覆盖全面,以上仅为潜在改进点。


📁 tests/test_asm_beautifier_formatting.py

🟡 Hardcoded column widths instead of constantstest_padding_caps_do_not_truncate_long_fields uses literal 30 and 40. If the cap constants change, the test silently breaks. Suggestion: reference MAX_LABEL_WIDTH / MAX_OPERAND_WIDTH (or whatever constants exist) so the test adapts automatically.

🟡 test_short_fields_use_minimum_widths relies on implicit comment additionbeautify_asm("ret\nadd a0,a1,a2") (no add_comments=False) assumes a comment is added to the ret line. ret_line.index("#") will raise ValueError if the default behavior changes. Suggestion: either pass add_comments=False and verify alignment without comments, or guard the assertion with a fallback.

💭 test_two_pass_alignment_normalizes_operands_and_preserves_comment — passes add_comments=False but still expects the original # sum comment to be preserved. If the parameter’s intent is “do not add extra comments”, the naming is ambiguous; consider clarifying the API or renaming the parameter.

💭 test_no_align_returns_original_newlines_and_whitespace — expects exact equality including \r\n. If the beautifier normalizes line endings under the hood, the test may fail on non‑Windows CI. Consider normalizing to \n before comparison or documenting the expected behavior.


📁 tests/test_asm_beautifier_integration.py

🔴 Blocker: 指令前导空格假设错误

test_declared_inline_function_label_is_written_separatelytest_ordinary_inline_label_is_also_written_separately 断言 not lines[-1].startswith(" ")。当 align=True(默认)时,指令会被缩进对齐,导致测试失败。
建议:使用 align=False 或检查 lines[-1].strip().startswith(...)


🔴 Blocker: 对 os.replace 的 patch 方式脆弱

test_beautify_file_does_not_replace_output_on_failure 通过 monkeypatch.setattr(asm_beautifier.os, "replace", ...) 模拟失败。如果实现改为 from os import replace 或使用 Path.replace,则 asm_beautifier.os 可能不存在或属性名不同,测试会崩溃。
建议:patch os.replacepathlib.Path.replace 更健壮,例如 monkeypatch.setattr("os.replace", fail_replace)


🟡 测试依赖内部实现常量

_standard_comment_column() 使用了 OPCODE_WIDTH_MINOPERANDS_WIDTH_MIN 等未公开常量。若这些常量调整,测试需同步更新。
建议:考虑使用更稳定的断言,例如检查各行注释列是否对齐,而非计算绝对列数。


🟡 独立注释对齐行为假设不确定

test_standalone_comment_aligns_while_unsafe_and_blank_lines_are_preserved 断言独立注释列与内联注释列相同,但美化器可能不移动独立注释。
建议:确认该行为是否属于预期特性;若不是,应放宽断言。


💭 测试过长可拆分

test_default_options_merge_comments_in_structured_program 包含 22 行断言,涉及多个独立行为。
建议:拆分为多个小测试,以提升隔离性和可读性。



⚠️ 未审查的文件

  • tests/test_parser_for_beautifier.py

@watney1024
watney1024 self-requested a review August 1, 2026 01:23
@SCOFRD SCOFRD changed the title 对topic5增加了设计与开发文档 topic5: docs Aug 3, 2026
@SCOFRD

SCOFRD commented Aug 10, 2026

Copy link
Copy Markdown
Author

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负责压力测试

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant