Keyboard shortcuts

Press ← or → to navigate between chapters

Press S or / to search in the book

Press ? to show this help

Press Esc to hide this help

3.2 代码审查——第二双眼睛看第一双的盲区

波音707的机翼计算书

1954年，波音707的机翼设计团队里有一条不成文的规矩：任何一张结构计算书，必须经过另一位工程师的独立验算才能签字盖章。这一规矩的起因来自一次险些发生的灾难：一位资深工程师在计算一个翼肋连接板的载荷时，正确地应用了公式，正确地查了材料手册，却遗漏了一个关键假设：他默认这块连接板装在机翼内部（干区），但实际上它暴露在机翼前缘（湿区），会经受雨水反复侵蚀。他的计算本身没有任何数学错误，但他的视野有一个盲区：他不知道这块板的具体安装位置。

另一位工程师在复核时，第一句话就是：“等等，这块板是装在哪里的？”

就这一句话，阻止了一个潜在的空中解体事故。

软件世界里，这种“计算正确但假设错误“的盲区每天都在发生。你看一段代码，逻辑没问题，语法没问题，但你没有意识到 CAN_Write() 这个函数在某个中断优先级下不可重入；你没有意识到这块 static 缓冲区在两个任务之间共享但没有任何保护；你没有意识到这片 AD 采样的滤波窗口刚好和发动机的一个振动周期重叠。

这些都不是 Bug，是盲区。 而盲区的本质特征是：你自己看不到它。

这就是代码审查（Code Review）存在的根本原因：不是找 Bug，是补盲区。

核心洞察：代码审查不是找 Bug，是补盲区。 “计算正确但假设错误“的缺陷，恰恰是你自己看不到的：不可重入的函数、无保护的共享缓冲区、与振动周期重叠的滤波窗口。盲区的本质特征是“自己看不到它”，因此只有第二双懂得领域的眼睛能发现它——它检查的是代码里隐含的、跨边界的假设。

为什么编译器、静态分析和测试都不能替代审查？

你可能会问：编译器不就能检查语法吗？静态分析不就能检查潜在错误吗？单元测试不就能验证逻辑吗？为什么还需要人眼看？

因为编译器、静态分析器和测试各自有各自的盲区：

工具能检查什么看不到什么
编译器语法、类型意图、上下文
静态分析模式匹配的错误特定于领域的错误假设
单元测试给定输入的输出正确性你没有写的测试用例
代码审查意图与实现的一致性（审查者自身的盲区）

编译器不知道 TIMEOUT_MS 设为 500 是对的还是错的，它只知道这是个合法的整数。静态分析器不知道这个 SPI 设备的时序要求是 10ns 还是 100ns，它只知道指针解引用是否安全。单元测试不知道你没有测试“中断在 UART 发送中途到来“这个场景。

只有另一双懂得这个领域的眼睛能判断：你写的代码，和你以为你写的代码，是不是同一个东西。

举个嵌入式 C 的真实例子：

/* 你写的代码： */
void Can_ProcessMessages(void)
{
    for (uint8 i = 0; i < can_msg_cnt; i++)
    {
        if (can_msg_buf[i].id == 0x18FEF100)  /* 诊断请求 */
        {
            Diag_HandleRequest(&can_msg_buf[i]);
        }
    }
}

编译器：✅ 语法正确，类型匹配。 静态分析：✅ 无越界风险（i 有界），指针非空（can_msg_buf 已验证）。 单元测试：✅ 模拟了 0x18FEF100 的 CAN 消息，Diag_HandleRequest 被正确调用。

代码审查者：“等一下，这个函数是在主循环里调用的还是中断里调用的？如果在中断里，Diag_HandleRequest 可能调用 NVM 写入函数，而 NVM 写入函数可能阻塞 50ms。你在中断里阻塞了 50ms，你知道这会导致什么吧？”

这就是代码审查的独特价值：它检查的是你代码里隐含的、跨边界的假设。

核心洞察：编译器、静态分析、测试各有盲区，唯独人眼能检查“意图与实现的一致性“。 编译器不知道 500ms 超时是否合理，静态分析器不知道 SPI 的时序要求，单元测试不知道你没写的场景。只有另一双懂领域的眼睛能判断：你写的代码，和你以为你写的代码，是不是同一个东西。

嵌入式 C 代码审查清单

不是所有的代码审查都是平等的。漫无目的地“看一看“效果很有限，审查者需要一个结构化的清单来系统性地扫描盲区。

以下是针对汽车嵌入式 C 的代码审查清单，按检查层级排列。这相当于结构工程师复核计算书时的“标准检查表“。

第一层：中断与并发安全（ISR Safety）

这是嵌入式 C 的第一大杀手。审查时对每一处中断服务函数问：

  • ISR 内是否有阻塞操作（循环等待、NVM 写入、大内存拷贝）？
  • ISR 内调用的所有函数是否都是可重入的（只使用栈变量或原子操作）？
  • ISR 与主循环共享的变量是否声明为 volatile 并且有适当的临界区保护？
  • 中断嵌套时，高优先级 ISR 是否会破坏低优先级 ISR 的中间状态？
  • volatile 是否只在真正需要的地方使用？（滥用 volatile 会抑制编译器优化并使意图模糊）

建筑类比：地震来的时候，大楼的每一层都承受不同的水平剪力。结构工程师必须检查地基和上部结构的连接节点，那里是应力最集中的地方。ISR 就是软件里的“连接节点“。

第二层：内存与栈

  • 所有动态内存分配（malloc）是否在初始化阶段完成？（运行时分配在汽车嵌入式上通常是禁止的）
  • 栈深度是否被显式评估过？（最坏情况调用链的栈使用量 < 配置的栈大小，且留有余量）
  • 递归调用是否存在？（在安全关键系统中，递归通常是禁止的）
  • 数组访问是否有边界检查？
  • 所有指针解引用前是否已检查非空？

建筑类比：一栋楼的活荷载（人、家具、设备）加上恒荷载（自身重量）不能超过地基承载力的某个百分比。内存和栈就是软件的“荷载“。

第三层：状态机与逻辑完整性

  • 状态机的每个状态对每个可能的事件都有定义好的处理（包括“不可能发生“但确实可能因硬件故障而发生的事件）？
  • 状态迁移是否原子化？（一个状态迁移涉及多个变量的修改时，中途是否可能被中断打断？）
  • 所有 switch 语句是否有 default 分支？
  • 所有 if-else if 链是否有最终的 else 兜底？
  • 错误路径是否被处理？（通信超时、校验失败、传感器返回异常值，这些路径虽然“不应该发生“，但必须被处理）

建筑类比：防火楼梯可能永远不被使用，但只要有一次真实火灾，它就是唯一的生路。错误处理路径就是软件的“防火楼梯“。

第四层：硬件交互

  • 硬件寄存器访问是否有适当的 volatile 修饰？（注意：某些编译器下硬件寄存器通过特定地址访问，volatile 是必需的）
  • 写入硬件寄存器后是否有必要的等待周期？（硬件不会立刻响应）
  • SPI/I2C/CAN 通信是否有时钟容错机制？（如果对方设备的时钟偏移 5%，通信会失败吗？）
  • ADC 采样值是否有滤波和合理性检查？（传感器故障时，原始值可能是满量程或零）
  • 看门狗是否正确喂狗？（喂狗的位置是否在健康检查之后，而不是在循环的任意位置？）

第五层：MISRA C 合规性

如果你的项目遵循 MISRA C:2012（多数汽车项目都如此），审查时额外关注：

  • Rule 2.2：没有死代码（Dead Code）。
  • Rule 8.3：声明与定义的类型一致。
  • Rule 11.3：指向不同对象类型的指针之间没有强制转换。
  • Rule 13.2：for 循环控制变量的值没有被循环体修改。
  • Rule 17.2：没有使用递归。
  • Directive 4.9：优先使用 const 或枚举而非 #define 来定义常量（函数式宏应避免）。

请注意，这些规则不是教条。MISRA 允许偏差（Deviation），但每一个偏差必须有文档化的理由和缓解措施。

核心洞察：漫无目的地看收效甚微，审查需要结构化清单来系统扫描盲区。 五层清单从最致命的 ISR 与并发安全，到内存与栈、状态机完整性、硬件交互、MISRA 合规，逐层逼近软件里的“连接节点“。真正危险的往往不是显而易见的错误，而是每个状态机里“不可能发生、却因硬件故障会发生“的路径。

PR（Pull Request）的最佳实践

在代码审查的实际操作层面，有两种极端的反模式：

反模式 A：巨型 PR，一个 PR 改了 38 个文件，涉及 6 个模块，总共 3200 行。审查者打开第一页就开始头疼，翻到第三页已经无法保持注意力。最终的结果要么是“粗略扫一眼就批准“，要么是审查者花一整天认真看，而这一整天本可以做其他工作。

反模式 B：零上下文 PR，PR 标题写着“fix“，描述为空，没有链接到需求或 Bug 追踪系统。审查者完全不知道这个改动是解决什么问题，也就不可能判断这个解决方案是否恰当。

小型 PR 的纪律

经验法则：一个 PR 不超过 400 行变更（新增 + 删除）。

400 行不是绝对的硬限制，但它是一个心理阈值。超过 400 行，审查者的注意力开始衰减。人的大脑在一段时间内只能有效跟踪有限数量的“变更-影响“关系。

把大功能拆成一系列小 PR 是一种技能，也是一种纪律：

不要：  PR #42 "实现 UDS 诊断服务全套功能" (2800 行)
应该：  PR #42 "实现 UDS Service 0x22 ReadDataByIdentifier" (320 行)
        PR #43 "实现 UDS Service 0x2E WriteDataByIdentifier" (280 行)
        PR #44 "实现 UDS Service 0x19 ReadDTCInformation" (450 行)

拆 PR 的技巧在于找到最小可审查单元。一个最小可审查单元的特点：

  1. 可以独立编译通过（或至少在逻辑上自洽）。
  2. 可以用一两句话清楚地描述“做了什么、为什么“。
  3. 如果需要，可以单独回滚而不破坏其他功能。

PR 描述：不只是写了什么，更是为什么

一个好的 PR 描述回答三个问题：

## 做了什么
为 ADC 驱动增加了 DMA 双缓冲模式，解决高采样率下数据丢失问题。

## 为什么这样做
当前单缓冲模式下，当 ADC 以 100kHz 采样时，DMA 传输完成的回调函数
处理数据花费约 15μs。在此期间新的采样值可能覆盖尚未处理的缓冲区数据。
改为双缓冲（Ping-Pong）后，一个缓冲区在填充时，另一个已被 DMA 传输
完成通知触发处理。

## 如何验证
- 单元测试：验证了双缓冲切换逻辑（test/ut/test_adc_dma.c）
- 集成测试：在目标硬件上以 100kHz 连续采样 60 秒，零数据丢失
- 标定数据：使用数据采集卡对比，采样值与参考值偏差 < 0.1%

## 关联
JIRA: AUTOSAR-1823
需求: TSR-ADC-014 (DMA 双缓冲模式)

审查反馈的语调：关于代码，不关于人

一段糟糕的审查评论：

“你怎么能这样写？这是典型的错误。你应该用互斥锁保护共享变量。回去重写。”

这段话攻击的是人（“你”），而不是代码。

一段好的审查评论：

“我看到 can_msg_cnt 在 ISR 和主循环中都被修改了。这两个上下文之间没有互斥保护。如果主循环在读取 can_msg_cnt 时被 CAN ISR 中断，并且 ISR 修改了该值，主循环会使用一个不一致的计数值。建议在读取前后加临界区保护，或者使用原子操作。”

“参考：《Understanding AUTOSAR》第 8.3 节关于 ISR 与 Task 间数据一致性的讨论。”

这段话的特点：

  1. 描述了具体的技术问题，不是评价个人能力。
  2. 解释了为什么这是个问题（不一致的计数值）。
  3. 给出了建议方案，不是单纯的否定。
  4. 引用了权威来源，让审查反馈有据可查。

审查文化最核心的原则是：代码不是你。代码是对你当前解决方案的一个描述，而这个描述可以被改进。 这和建筑师审图一样，审图人是在用第二双眼睛确认结构的安全性。

核心洞察：PR 的可审查性与质量成正比：小到能看透，描述到能懂，语气对事不对人。 超过 400 行注意力开始衰减；描述要回答“做了什么、为什么、如何验证“；反馈要描述具体技术问题、解释原因、给出建议、引用依据。审查文化最核心的一条：代码不是你，它只是一个可以被改进的描述。

审查流程的节奏：不要挤压在最后

许多团队的代码审查流程是这样的：开发者在功能分支上写了两个星期的代码，周五下午提交 PR，审查者在周一上午集中看，发现大量问题，开发者又花两天修改，审查者再花一天复查，最终合并。

这个流程的致命问题是反馈延迟。开发者在写完代码两周后才得到审查反馈，他已经忘记了很多设计决策的细节。审查者面对的是一个已经成熟的解决方案，提出结构性修改意见的成本极高。

健康的节奏：功能分支的生命周期短（1-2天），PR 小（<400行），审查在 PR 提交后24 小时内完成。

这等于在说：审查是开发过程中的同步活动，而不是开发的“下一阶段“。就像结构工程师画完一层柱网图立即请同事复核，而不是画完整栋楼再拿过去，等到那时发现问题，改图成本已经爆炸了。

核心洞察：审查是开发中的同步活动，不是开发后的“审核关卡“。 写两周再审的最大代价是反馈延迟：开发者已忘记设计细节，结构性修改的成本已爆炸。健康的节奏是短分支、小 PR、24 小时内完成审查——就像画完一层柱网立即请同事复核，而不是盖完整栋楼再返工。

本篇小结

  • 代码审查不是找 Bug，是补盲区。编译器、静态分析、测试各有盲区，审查填补它们之间的空白。
  • 嵌入式 C 审查清单五层：ISR 安全 → 内存与栈 → 状态机完整性 → 硬件交互 → MISRA 合规。
  • PR 不超过 400 行。拆大功能为最小可审查单元。PR 描述回答“做了什么、为什么、如何验证“三个问题。
  • 审查反馈关于代码，不关于人。用具体的技术语言描述问题、解释原因、给出建议、引用依据。
  • 审查是开发过程中的同步活动，不是开发之后的“审核关卡“。24 小时内完成审查。
  • 审查记录是团队的知识遗产。它记录的是设计决策。

【下集预告】：代码审查靠的是人眼，但人眼也会疲劳，也会遗漏。建筑行业有一项技术叫“有限元分析“，计算机模型自动计算结构每一处的应力分布，找出工程师可能遗漏的应力集中点。软件工程里，这项技术叫静态分析。

你的 CAN 总线关闭恢复逻辑通过了代码审查。但审查者和你都没有注意到，在某个极端的引脚配置下，CAN 控制器的初始化顺序与你假设的不同。这个缺陷只有在特定的硬件板本和特定的上电时序下才会触发。审查者是人，人需要工具的辅助。下一节，让机器当你的第三只眼。