代码审查常见问题解答

代码审查的目的是什么?

代码审查是我们验证补丁设计和实现的基本机制。它还有助于我们在众多黑客和 Mozilla 的各个模块之间保持设计和实现实践的一致性。

当然,代码审查并非瞬时完成,因此系统中存在一定的延迟。我们一直在寻找减少等待时间的方法,同时允许审查者自己进行大量黑客攻击。我们没有完美的系统,将来也不会拥有。它仍在发展,因此如果您有任何建议,请告诉我们。

Mozilla 过去曾使用过“超级审查”的概念,但2018 年达成共识 决定停止此流程。

谁必须审查我的代码?

您必须获得模块所有者或代码将被签入的模块的指定“同行”的批准(“r={{ mediawiki.external(‘name’) }}”)。如果您的代码影响多个模块,则通常您应该获得每个受影响模块的所有者或指定同行的“r={{ mediawiki.external(‘name’) }}”。我们试图在这里保持合理性,因此我们没有关于何时必须获得每个模块所有者批准的绝对规则。例如,对字符串类或在许多模块中显示的文本进行的树级更改通常不会由每个模块所有者进行审查。

您可能也希望征求其他人的意见。

审查者会寻找什么?

审查的重点在于补丁的设计、实现、在解决既定问题方面的实用性以及在其模块中的适用性。审查者应该是该问题领域具有专业知识的人。审查者还可以利用其其他专业领域并对其他可能的改进发表评论。审查者对改进代码可能提出的评论没有内在限制。

审查者可能会查看代码的以下方面

  • “目标”审查:正在解决的问题实际上是 Bug 吗?补丁是否解决了根本问题?

  • API/设计审查。由于 API 定义了模块之间的交互,因此需要特别注意。审查对于保持 API 的平衡和针对性,以及避免过于具体或过度设计尤其重要。有一个WebIDL 审查清单。当 API 将要公开给 Web 时,还有一些应发送的电子邮件模板,以及关于此 Wiki 页面上命名的一般指南。

  • 可维护性审查。无法阅读的代码无法维护。如果审查者必须询问代码片段的目的,那么它可能没有得到充分的记录。代码是否遵循编码风格?使用现代 C++ 特性(如 auto)时要小心审查代码。

  • 安全审查。设计是否使用了安全概念,例如输入清理器、包装器和其他技术?此代码是否需要其他安全测试,例如模糊测试或静态分析?

  • 集成审查。此代码是否与其他模块正常工作?它是否已正确本地化?它是否有服务器依赖项?它是否有用户文档?

  • 测试审查。是否有测试来验证正确功能?是否有测试来检查错误条件和操作过程中可能发生的错误输入?

  • 性能审查。此代码是否已进行分析?您确定它不会对其他代码的性能产生负面影响吗?

  • 许可证审查。代码是否遵循代码许可规则

如何查看审查状态?

当补丁通过审查时,您将在 Phabricator 修订版的标题下方看到绿色显示的“已接受”。在 Bugzilla(已弃用,取而代之的是 Phabricator)中,这由错误报告中附件表中的“{{ mediawiki.external(‘name’) }}:review+”指示。如果审查失败,您将在修订版的顶部看到红色显示的“需要修订”,或者在 Bugzilla 中看到“{{ mediawiki.external(‘name’) }}:review-”。大多数情况下,当审查者设置审查标志时,他们还会在 Bug 中添加一条评论来解释审查。