LLVM 代码审查政策与实践

LLVM 的代码审查政策与实践有助于维护整个项目的高代码质量。具体来说,我们的代码审查流程旨在:

  • 提高可读性和可维护性。

  • 提高稳健性并防止引入缺陷。

  • 为每个提议的变更充分利用其他贡献者的经验。

  • 通过社区领导者的指导,帮助培养和发展新的贡献者。

所有贡献者都必须理解我们的代码审查实践并参与代码审查过程。

通用政策

哪些代码应该被审查?

所有开发者都必须在重大更改提交到仓库之前进行审查。

代码是否必须在提交前进行审查?

代码可以在提交前或提交后进行审查。我们期望重要的补丁在提交前进行审查。较小的补丁(或开发者拥有组件的补丁)如果符合可能的社区共识要求(适用于所有补丁批准),则可以在显式审查之前提交。在存在任何不确定性的情况下,补丁应在提交前进行审查。

请注意,负责补丁的开发者也负责进行所有必要的与审查相关的更改,包括在任何提交后审查期间要求的更改。

代码可以在提交后进行审查吗?

鼓励提交后审查,并且可以使用下面详细介绍的任何工具来完成。强烈期望作者及时回应提交后反馈并解决问题。否则,补丁可能会被回退

如果社区成员对最近的提交表示担忧,并且这种担忧足以在提交前审查期间引起讨论(包括围绕需要更多设计讨论),他们可以要求原始作者回退,原始作者有责任及时回退补丁。开发者经常意见不一致,但站在要求更多审查的开发者一边可以防止对树中的代码产生任何挥之不去的异议。这并不表示补丁作者有任何过错,这是我们提交后审查实践固有的。回退补丁确保设计讨论可以在不阻止其他开发的情况下进行;一旦问题得到解决,补丁完全有可能最终以基本相同的形式重新应用。

在重新提交之前,补丁通常应接受进一步审查。发现问题的社区成员应积极参与审查。如果问题是由构建机器人发现的,则期望拥有与构建机器人类似硬件访问权限的社区成员参与审查。

请注意:提交后反馈的标准不高于提交前反馈的标准。不要不必要地延迟提供反馈。但是,如果您在代码提交后发现一些您会在提交前评论的内容(如果您早些注意到),请随时提供反馈。

话虽如此,如果自原始更改提交以来已经过了相当长一段时间,那么创建新补丁来解决问题可能比评论原始提交更好。例如,原始补丁作者可能不再是项目的活跃贡献者。

代码审查使用哪些工具?

提交前代码审查在 GitHub 上使用 Pull Requests 进行。请参阅 GitHub 文档。

何时需要 RFC?

有些更改对于仅仅进行代码审查来说太重要了。应该更改 LLVM 语言参考(例如,添加新的目标无关的内在函数)、在 Clang 中添加语言扩展等等的更改,首先需要在 LLVM 讨论论坛上发起 RFC(征求意见稿)主题。对于承诺对用户和/或下游代码库产生重大影响的更改,审查者可以要求在进行代码审查之前达成 RFC 共识。话虽如此,发布初始补丁可以帮助进行 RFC 讨论。

代码审查工作流程

代码审查可能是一个迭代过程,该过程持续到补丁准备好提交为止。具体来说,一旦补丁被发送出去进行审查,它需要明确的批准才能提交。不要假设沉默批准,或者以截止日期征求对补丁的异议。

注意

如果您使用 Pull Request 的目的不是审查(例如:提交前 CI 结果、方便的基于 Web 的回退等),请使用 skip-precommit-approval 标签标记 PR。

确认所有审查者的反馈

补丁作者应确认审查者的所有评论。通常期望建议的更改将被纳入补丁的未来版本中,除非作者和/或其他审查者可以阐明充分的理由不这样做(然后审查者必须同意)。如果新补丁没有解决所有未解决的反馈,作者应在提供更新的补丁时明确说明。当使用基于 Web 的代码审查工具时,此类注释可以在“Diff”描述中提供(这与用于提交消息的“Differential Revision”的描述不同)。

如果您在代码审查中提出更改建议,但不希望该建议被如此强烈地解释,请明确说明。

注意

在回应审查者评论后,按 重新请求审查 以引起审查者对 Pull Request 的注意。

旨在高效利用每个人的时间

旨在限制审查过程中的迭代次数。例如,当建议更改时,如果您希望作者在代码的其他位置进行一组类似的更改,请解释请求的更改集,以便作者可以一次进行所有更改。如果补丁在获得批准之前需要多个步骤(例如,拆分、重构、发布来自特定性能测试的数据),请尽可能提前解释其中的许多步骤。这允许补丁作者和审查者最有效地利用他们的时间。

LGTM - 如何接受补丁

当审查者接受补丁时,补丁被批准提交,这几乎总是与包含文本“LGTM”(代表 Looks Good To Me,即“在我看来不错”)的消息相关联。

除非 pull request 有要求的审查者,否则只需要一位审查者的批准。如果是这种情况,您必须获得所有这些审查者的批准。

当提供不合格的 LGTM(批准提交)时,审查者有责任审查所有之前的讨论和所有审查者的反馈,确保所有反馈都已得到解决,并且所有其他审查者几乎肯定会对补丁获得批准感到满意。如果不确定,审查者应提供有条件的批准,(例如,“LGTM,但请等待 @someone, @someone_else”)。如果您相当确定某个特定的社区成员希望进行审查,即使该人尚未这样做,您也可以这样做。

如果在接受后提供其他反馈(由同一审查者或另一审查者),作者应运用他们的最佳判断来决定该反馈是否可以在不评论的情况下纳入更改(例如,拼写错误)或是否需要进一步的审查讨论。更实质性的评论(例如,关于设计的评论)通常需要进一步讨论。如果不确定,请咨询审查者。

请注意,如果审查者要求特定的社区成员进行审查,并且一周后该社区成员尚未回应,请随意 ping 补丁(字面意思是在补丁上提交带有“Ping.”一词的评论),或者,向原始审查者寻求进一步的建议。

如果最近发布的补丁很可能其他人也想审查,特别是如果可能存在异议,但还没有其他人这样做,那么提供有条件的批准也是礼貌的(例如,“LGTM,但请等待几天,以防其他人希望审查”)。如果很快收到批准,补丁作者也可以选择等待再提交(对于非平凡的补丁,这当然被认为是礼貌的)。特别是考虑到我们社区的全球性,这个等待时间应至少为 24 小时。另请注意周末和主要节假日。

我们的目标是确保社区对设计决策和重要的实现选择达成共识,审查者在为补丁提供总体批准时,其责任之一是合理地确定这种共识是否存在。如果您对社区不够熟悉以至于无法了解,那么您不应该提供最终的提交批准。提供最终批准的审查者应具有 LLVM 项目的提交权限。

每个补丁都应由至少一位受更改影响的项目领域的资深技术专家进行审查。

拆分请求和有条件接受

审查者可以请求将补丁的某些方面拆分成单独的补丁以进行独立审查。审查者也可以接受补丁,条件是作者提供后续补丁来解决某些特定问题或疑虑(尽管任何已提交的补丁都不应使项目处于损坏状态)。此外,审查者可以接受补丁,条件是作者在提交前应用一组小的更新,并且在适用的情况下,审查者这样做是礼貌的。

不要无意中阻止审查

如果您审查了一个补丁,但不希望审查过程因您的批准而受阻,请明确说明。出于礼貌,我们通常会等待提交补丁,直到所有审查者都满意为止,如果您不打算及时再次查看补丁,请在审查中传达这一事实。

谁可以/应该审查代码?

非专家应该审查代码

您无需成为代码库某些领域的专家即可审查补丁;可以随意询问某段代码正在做什么。如果您不清楚发生了什么,那么您可能不是唯一一个不清楚的人。请记住,拥有只有少数人才能很好地理解的组件不符合社区的长期最佳利益。额外的注释和/或测试用例通常可以提供帮助(并且要求在测试用例中添加注释也很好)。

此外,鼓励作者将问题解释为重新审视相关代码可读性的理由。结构性更改或其他注释可能是合适的。

如果您是 LLVM 社区的新手,您可能还会发现此演示文稿很有帮助:.. _How to Contribute to LLVM, A 2019 LLVM Developers’ Meeting Presentation: https://youtu.be/C5Y977rLqpw

新贡献者增加代码库知识的一个好方法是审查代码。审查代码并明确地将批准决定推迟给他人是完全可以接受的。

专家应该审查代码

如果您是受提议补丁影响的编译器领域的专家,那么强烈建议您审查代码。如果您是相关的维护者,并且没有其他专家审查补丁,您必须帮助安排专家审查补丁或亲自审查。

代码审查、速度和互惠

有时代码审查会比您希望的要花费更长的时间,尤其是对于较大的功能。加快补丁审查时间的常用方法是:

  • 审查其他人的补丁。如果您伸出援手,每个人都会更愿意为您做同样的事情;善意是我们的货币。

  • Ping 补丁。如果紧急,请说明为什么对您来说让此补丁落地很重要,并每隔几天 ping 一次。如果不紧急,通常的礼貌 ping 频率为一周。请记住,您正在向其他专业开发人员索取宝贵的时间。

  • 在 Discord 上寻求帮助。Discord 上的开发者将能够直接帮助您,或者告诉您谁可能是合适的审查者。

  • 将您的补丁拆分为多个较小的、相互构建的补丁。您的补丁越小,有人快速浏览它的可能性就越高。这样做时,最好在系列中每个补丁的标题中添加“[N/M]”(对于 1 <= N <= M),以便清楚地表明存在顺序以及该顺序是什么。

开发者应作为审查者和作者参与代码审查。如果有人好心地审查您的代码,您应该为其他人回报恩情。请注意,欢迎任何人审查补丁并提供反馈,但补丁的批准应与上述政策一致。