diff --git a/CODEOWNERS b/CODEOWNERS new file mode 100644 index 000000000000..520786647093 --- /dev/null +++ b/CODEOWNERS @@ -0,0 +1,11 @@ +# Global owners. +* @carbon-language/admin-team + +# The core team owns the core language and project documentation. +/docs/ @carbon-language/core-team + +# The review managers own updates to the proposals. +/proposals/ @carbon-language/review-managers + +# This directory contains infrastructure and support scripts +/src/ @carbon-language/infra-team diff --git a/docs/project/code_review.md b/docs/project/code_review.md new file mode 100644 index 000000000000..afea8020347e --- /dev/null +++ b/docs/project/code_review.md @@ -0,0 +1,469 @@ +# Code review + + + +## Table of contents + + + +- [High level goals of code review](#high-level-goals-of-code-review) +- [What requires review?](#what-requires-review) +- [Who should review?](#who-should-review) +- [GitHub pull request mechanics](#github-pull-request-mechanics) +- [Code author guide](#code-author-guide) + - [Write good change descriptions](#write-good-change-descriptions) + - [First line](#first-line) + - [Body](#body) + - [Make small changes](#make-small-changes) + - [Responding to review comments](#responding-to-review-comments) + - [Responding to questions or confusion](#responding-to-questions-or-confusion) + - [Understand the feedback in the comments](#understand-the-feedback-in-the-comments) +- [Code reviewer guide](#code-reviewer-guide) + - [How quickly should you respond to a review request?](#how-quickly-should-you-respond-to-a-review-request) + - [What should be covered by a review?](#what-should-be-covered-by-a-review) + - [Writing review comments](#writing-review-comments) + - [Approving the change](#approving-the-change) +- [Resolving an impasse or conflict](#resolving-an-impasse-or-conflict) +- [Escalation](#escalation) + + + +## High level goals of code review + +Code review serves several goals in the Carbon project. It directly improves the +correctness, clarity, and consistency of the code. These improvements range from +the high-level functionality down through the design and implementation details. +It also promotes team ownership and spreads knowledge across the team. + +More detailed discussions can be found in chapter 9 "Code Review" of the book +_[Software Engineering at Google](https://www.amazon.com/Software-Engineering-Google-Lessons-Programming/dp/1492082791)_ +and chapter 21 "Collaborative Construction" in +_[Code Complete: A Practical Handbook of Software Construction](https://www.amazon.com/Code-Complete-Practical-Handbook-Construction/dp/0735619670/)_. +However, these details aren't essential to understanding code review and how it +works in the Carbon project. All of the important details are provided in the +project documentation. + +## What requires review? + +Every change to Carbon's repositories requires code review. Even formal +[evolution decisions](evolution.md) which have been approved should have their +specific changes to the repository reviewed. + +Many changes to Carbon repositories may _only_ require code review. Typically, +these include bug fixes, and development or documentation improvements clearly +in line with accepted designs. It may in some rare cases extend to exploring +experimental or prototype directions whose design is under active consideration. + +The term "code review" in the Carbon project is not only about "code". We expect +changes to any files to be reviewed, including documentation and any other +material stored in the repository. + +## Who should review? + +Everyone should feel free to review Carbon changes. Even providing small or +partial review can be a good way to start contributing to Carbon. Contributors +with specific domain expertise or familiarity should also try to provide review +on changes touching relevant parts of the project. + +Additionally, at least one _code owner_ of any file changed needs to review that +change. The code owners and what files they are responsible for are defined +using the +[`CODEOWNERS`](https://help.github.com/en/github/creating-cloning-and-archiving-repositories/about-code-owners#codeowners-syntax) +file in the root of the repository. Pull requests will automatically request +reviewers based on this file and enforce that these reviews take place. + +While we do encourage people interested in contributing to Carbon by reviewing +changes to do so, we also suggest not overloading a single review. It can be +daunting for the author of a change to get feedback from a large number of +reviewers, and so we suggest keeping the number of reviewers reasonably small. + +Any reviews that explicitly request changes should be addressed, either with the +changes or an explanation of why not, before a pull request is merged. Further, +any code owners who have requested changes should explicitly confirm they're +happy with the resolution before the change is merged. + +When a team gives an affirm decision on an [evolution proposal](evolution.md), +each team member should explicitly note any of their comments on the pull +request that, while not blocking the _decision_, still need to be resolved as +part of code review prior to it being merged. These might, for example, be +trivial or minor wording tweaks or improvements. Otherwise, the decision is +assumed to mean the prior review comments from members of that team are +addressed; the author is free to merge once the pull request is approved, +possibly with a code review separate from the proposal's review. + +## GitHub pull request mechanics + +Carbon uses GitHub pull requests for code review, and we recommend some +mechanical best practices to most effectively navigate them. + +- Be aware that the main thread of pull request doesn't support threaded + discussions or "resolving" a comment. + - If either of those would be useful, you'll probably want to comment on a + file. + - You can quote comments in the main conversation thread in a reply by + clicking the three-dot menu on the original comment and selecting "Quote + reply". +- If you will want to comment on files, don't comment in the pull request + conversation. + - Always go to the `Files Changed` tab. + - Make any in-file comments needed, but add them to a pending review + rather than sending them directly. + - Finish the review and add any top-level review comments there. +- If you are a code owner who will be providing approval for the change, then + make sure to mark a review as requesting changes when you want the author to + begin addressing your comment. Only use the "comment" review state if you + are still in the process of reviewing and don't expect the author to begin + working on further changes. + - If you are not a code owner asked to approve, use the difference between + a comment and requesting a change to help the author know whether to + circle back with you before landing the pull request if the relevant + owner(s) approve it. +- Don't reply to in-file comment threads in the conversation view, or with + direct single reply comments. + - Add all replies to in-file comment threads using the `Files Changed` tab + and by adding each reply to a new review, and posting them as a batch + when done. + - You can get to the appropriate `Files Changed` tab by clicking on the + change listed in the conversation view with the incoming set of in-file + comments. + - This flow ensures an explicit update in the overall pull request that + can help both the author and other reviewers note that new replies have + arrived. +- Don't reply to an in-file comment and then mark it as resolved. No one will + see your reply as the thread will be hidden immediately when marked as + resolved. + - Generally, the person who started the comment thread should mark it as + resolved when their comments are sufficiently addressed. If another + reviewer is also on the thread and should also agree, just state that + you're happy and the last reviewer can mark it resolved. + - Trivially resolved threads can just be marked as "resolved" without + further update. Examples: a suggested change that has been successfully + applied, or a thread where the relevant reviewers have clearly indicated + they're happy. + +## Code author guide + +The goal of an author should be to ensure their change improves the overall +code, repository, and/or project. Within the context of code review, the goal is +to get a reviewer to validate that the change succeeds at this goal. That +involves finding an effective reviewer given the particular nature of the +change, helping them understand the change fully, and addressing any feedback +they provide. + +### Write good change descriptions + +The change description in the pull request is the first thing your reviewers +will see. This sets the context for the entire review, and is very important. + +#### First line + +The first line of a commit, or the subject of the pull request, should be a +short summary of specifically what is being done by that change. It should be a +complete sentence, written as though it was an order. Try to keep it short, +focused, and to the point. + +#### Body + +The description body may need to explain several important aspects of the change +to provide context for the reviewer when it isn't obvious from the change +itself: + +- The problem being solved by the change. +- Why the approach taken is the best one. +- Any issues, concerns, or shortcomings of the approach. +- Any alternatives considered or attempted. +- Relevant supporting data such as examples or benchmarks. + +Try to anticipate what information the reviewer of your change will need to have +in order to be effective. Also consider what information someone else will need +a year in the future when doing archaeology on the codebase and they come across +your change without any context. + +### Make small changes + +Small changes have many benefits: + +- Faster review. +- More thorough review. +- Easier to merge. +- Easier to revert if needed. + +The ideal size of a change is as small as possible while it remains +self-contained. It should address only _one thing_. Often, this results in a +change only addressing _part_ of a feature rather than the whole thing at once. +This makes work more incremental, letting the reviewer understand it piece by +piece. It can also make it much easier to critically evaluate whether each part +of a feature is adequately tested by showing it in isolation. + +That said, a change should not be so small that its implications cannot easily +be understood. It is fine to provide the reviewer context or a framework of a +series of changes so they understand the big picture, but that will only go so +far. It is still possible to shrink a change so much that it becomes nonsensical +in isolation. For example, a change without appropriate tests is not +self-contained. + +You may want to use a set of stacked pull requests rather than a single, larger +pull request in order to keep changes easy to review. + +> TODO: link to the stacked pull request documentation when available. + +### Responding to review comments + +Many comments have easy and simple responses. The easiest is **"Done"**. When +the comment is a concrete suggestion that makes sense and you implement it, you +can simply let the reviewer know their suggestion has been incorporated. If the +_way_ you implemented the suggestion might need clarification, add that as well. +For example, consider mentioning tweaks to the suggestion or when the suggestion +was applied in more places. + +When a suggestion from the reviewer is explicitly optional, you may also have a +simple response that you're not going to make the change. This is totally fine +-- if it weren't, the reviewer shouldn't have listed it as optional -- but it +may be helpful to explain your reasoning to the reviewer so they understand +better why the optional suggestion didn't make sense to you. + +Sometimes comments, even optional ones, center around slight differences or +preferences around the code. Consider that the reviewer may be a good proxy for +future readers. If the suggestion is essentially equivalent to your original +code, consider adopting it as it may make the code easier to read for others. +But if you feel the current choice is _better_, even if only slightly, stand up +for yourself and keep it. The reviewer can always push for a change and justify +it if needed. + +For non-optional comments, this section provides several suggestions on how best +to make progress. If none of these work, you may need to +[resolve an impasse or conflict](#resolving-an-impasse-or-conflict). + +#### Responding to questions or confusion + +Some comments in code review will be questions or confusion as the reviewer +tries to understand the code in question or why a particular approach was used. +Don't assume that questions are a request for a change. Reviewers should be +explicit if they think a change is needed rather than merely asking questions. +You should assume a question or confusion is something which only needs to be +clarified. + +However, when responding to a question or confusion, consider making changes to +improve clarity in addition to responding within the review, such as by adding +comments or changing code structure. The reviewer may not be the last person to +need more clarity, and you should use their comments as a signal for +improvement. Once done, the review response should typically focus on verifying +that the clarifications made in the code are sufficient for the reviewer. + +#### Understand the feedback in the comments + +At times, review comments may be confusing or frustrating for you. While this is +something we always want reviewers to minimize, it will still happen at some +times and to some degree. It helps to remember that the goal of the review is to +ensure the change results in the project improving over time. + +If the review comment doesn't make sense, ask the reviewer to help you +understand the feedback better. If it isn't constructive or doesn't seem to +provide any meaningful path forward, ask the reviewer to provide this. Making +comments both clear and constructive are part of the reviewers' +responsibilities. + +Once there is a clear and effectively communicated comment that you understand, +it may still feel wrong or like it is unnecessarily blocking your progress. It +is important to try to step back in this situation and, no matter how certain +you are, genuinely consider whether there is valuable feedback. You should be +asking yourself whether the reviewer might be correct, potentially in an +unexpected or surprising way. If you can't decide this definitively, you may +need to work to get a deeper understanding. + +If you are confident that the reviewer's comment is incorrect, that is _OK_. The +reviewer is also only human and is certain to make mistakes and miss things. The +response needs to try to explain what it is that leads you to be confident in +your assessment. Lay out the information you have and how you are reasoning +about the issue to arrive at the conclusion. Try not to make assumptions about +what the reviewer knows or why they made the comment. Instead, focus on +surfacing explicitly your perspective on the issue. + +These parts of a review will often be a discussion and may need to iterate a few +times. That isn't intrinsically bad, but try to make sure that it doesn't result +in reiterating positions or repeating things. Make sure the discussion is +_progressing_ towards deeper understanding and recognize when you reach an +impasse or conflict and shift strategy to +[resolve that](#resolving-an-impasse-or-conflict). It is also useful to avoid +long delays between these iterations. Consider discussing over Discord chat or +scheduling a quick video chat on the specific issue. This can avoid multi-hour +-- or multi-day -- round trips. + +## Code reviewer guide + +The specific goal for a particular review should always be to ensure that the +overall health of the code, repository, and/or project improves over time. This +requires that contributions _make progress_ -- otherwise, nothing can improve. +However, the review should ensure that quality of changes does not cause the +health of the project to decrease over time. + +The primary responsibility for ensuring that code review remains constructive, +productive, and helpful resides in the _reviewer_. As a reviewer, you are in a +position of power and asked to critique the authors hard work. With this power +comes responsibility for conducting the review well. + +### How quickly should you respond to a review request? + +Try to respond to code review requests as soon as you can without interrupting a +focused task. At the latest, the next day you are working on the project. Note +that the review isn't expected to necessarily be complete after a single review. +It is more valuable to give reasonably quick but partial feedback than to delay +feedback in order to complete it. If leaving partial feedback, make it clear to +the author which parts are covered and which you haven't gotten to yet. + +Large changes are especially important to give incremental feedback on in order +to do so in a timely fashion. One of the first things to consider with large +changes is whether it can be split apart into smaller changes that are easier to +review promptly. + +This timeliness guidance doesn't apply to the higher-level +[evolution process](evolution.md) reviews. Evaluating those proposals will often +require a larger time investment and have their own timelines spelled out in the +process. Here, we are talking about simply reviewing changes themselves +orthogonally to any evolutionary discussion and evaluation. + +### What should be covered by a review? + +Things to consider and evaluate when reviewing changes: + +- Is the code well designed? + - Is the resulting functionality, including its interface, good for the + users of the code? + - Does the resulting design facilitate long-term maintenance? + - Can the code be simplified? Is there unnecessary complexity? + - Are things being implemented that aren't yet needed and only _might_ be + needed in the future? +- Is the code free of bugs and well tested? + - Is memory safely managed? + - Is any parallel or concurrent programming done safely? + - Do unit tests cover relevant behaviors and edge cases? + - Do any integration tests need to be extended or added? + - Do any fuzz tests need to be extended or added? + - Are any tests well designed to be both thorough but also maintainable + over time? +- Is the code easy to read? + - Are the names used in the code clear? + - Are all important or non-obvious aspects of the code well commented? Do + the comments focus on _why_ instead of _what_? + - Is there appropriate high level documentation for the change? + - Does the change adhere to all relevant style guides? + - Is the change consistent with other parts of the project? + +### Writing review comments + +These are general guidelines for writing effective code review comments: + +- **Be kind.** Detailed review, especially in an open source project, can be + stressful and difficult for the author. As a reviewer, part of the job is to + ensure the review experience ends up positive and constructive for the + author. +- **Stay constructive.** Focus your comments on suggesting specific ways to + improve the change. If you need to explain why an improvement is necessary, + focus on objective ways the improvement helps and avoid both subjective + assessments and anchoring on problems with the current state. +- **Explain why.** It is important for the author to understand not merely the + mechanical suggested change but what motivates it and why it matters. This + may help clear up misunderstandings, help the suggestion be understood and + applied more effectively, and allow internalizing improvements for future + contributions. +- **Provide a path forward.** The author needs to understand what they will + need to do to respond to your comments. For example, always provide + alternatives when commenting that the current approach won't work. + +Keep in mind that the goal is to improve the overall health of the code, +repository, and/or project over time. Sometimes, there will be pushback on +review comments. Consider carefully if the author is correct -- they may be +closer to the technical issues than you are and may have important insight. Also +consider whether the suggestion is necessary to achieve the overall goal. If the +suggestion isn't critical to make the change an overall improvement, it may be +fine for it to move forward as-is. + +As with all communication in the Carbon project, it is critical that your +comments are not unkind, unwelcoming, angry, ad-hominem attacks, or otherwise +violating our community's [code of conduct](/CODE_OF_CONDUCT.md). + +### Approving the change + +Be explicit and unambiguous at the end of your review. Select "Approve" when +submitting the review to mark this in GitHub. You can always include a message, +often "LGTM" or "Looks Good To Me" is often used. If you don't feel like you're +in a position to approve the change and are simply helping out with review +feedback, make that explicit as well. You should set the review to a "Comment" +in GitHub, but also state this explicitly in the message since this is the +default and doesn't indicate that your feedback _is_ addressed. For example, say +that "my comments are addressed, but leaving the final review to others" to +clearly indicate that you're happy but are deferring the decision to others. If +you are a code owner and deferring to someone else, it is essential to suggest +specific other reviewers. Otherwise, we risk all the code owners assuming +another is going to approve the change. + +An important technique to make progress, especially with different working hours +and timezones, is to approve changes even with outstanding comments. For +example, if the comments you have are straightforward and have unambiguous fixes +or suggested edits, you should give an LGTM with those comments addressed. The +author can always come back to you if they have questions, and we can always +revert changes if the resolution for some reason diverges wildly from your +expectations. + +## Resolving an impasse or conflict + +At some point, a review may reach an impasse or a genuine conflict. While our +goal is always to resolve these by building consensus in review, it may not be +possible. Both the author and any reviewers should be careful to recognize when +this point arrives and address it directly. Continuing the review is unlikely to +be productive and has a high risk of becoming acrimonious or worse. + +There are two techniques to use to resolve these situations that should be tried +early on: + +1. Bring another person into the review to help address the specific issue. + Typically they should at least be a code owner, and may usefully be a member + of the [core team](groups.md#core-team). + +2. Ask the specific question in a broader forum, such as Discord Chat, in order + to get a broad set of perspectives on a particular area or issue. + +The goal of these steps isn't to override the author or the reviewer, but to get +more perspectives and voices involved. Often this will clarify the issue and its +trade-offs, and provide a simple resolution that all parties are happy with. +However, in some cases, the underlying conflict isn't actually addressed. While +there is a desire to generally bias towards the direction of the code owners +during reviews, reviews should _not_ turn into a voting process. The reason for +proceeding in a specific direction should always be explained sufficiently that +all parties on the review are satisfied by the explanation and don't feel the +need to escalate. + +Fundamentally, both reviewers and the author need to agree on the direction to +move forward. If reaching that agreement proves impossible, the review should be +[escalated](#escalation). If you feel like an escalation is needed in a review, +be explicit and clear in requesting it. There is nothing bad about going through +this process, but it should only occur when needed and so it helps to be very +clear. + +Once the impasse or conflict is addressed, it is _essential_ to commit to that +direction. It can be especially difficult for the author to accept a direction +that they initially disagree with and make changes to their code as a result. An +essential skill is the ability to +[disagree and commit](https://en.wikipedia.org/wiki/Disagree_and_commit). + +## Escalation + +At the explicit request of any member of the [core team](evolution.md#core-team) +or to resolve any fundamental impasse in a review, the change should move to the +formal [evolution process](evolution.md#evolution-process). Ultimately, the +Carbon project [governance](evolution.md#governance-structure) structure is +always available as an escalation path. + +Before escalating an impasse or conflict in code review, try asking another +reviewer to help resolve the issue or bridge any communication gaps. Consider +scheduling a quick video chat to discuss and better understand each others' +concerns and position. + +Note that the formal evolution process is heavyweight and relatively slow. The +expectation is that this is rarely used and only to resolve serious and severe +disagreements. If this becomes a more common problem, lighter weight processes +may be needed to help ensure a reasonable rate of progress. diff --git a/proposals/README.md b/proposals/README.md index 27d65a25dd95..bd4a213e364e 100644 --- a/proposals/README.md +++ b/proposals/README.md @@ -25,6 +25,7 @@ request: - [0029 - Linear, rebase, and pull-request GitHub workflow](p0029.md) - [Decision](p0029_decision.md) +- [0042 - Create code review guidelines](p0042.md) - [0044 - Proposal tracking](p0044.md) - [Decision](p0044_decision.md) - [0051 - Goals](p0051.md) diff --git a/proposals/p0042.md b/proposals/p0042.md new file mode 100644 index 000000000000..110053b5ac04 --- /dev/null +++ b/proposals/p0042.md @@ -0,0 +1,135 @@ +# Create code review guidelines + + + +[Pull request](https://github.com/carbon-language/carbon-lang/pull/42) + +## Problem + +Carbon should ensure that all checked-in changes to the repository are properly +code reviewed, and that the process for code review is effective across a number +of dimensions: + +- Ensure high quality of code, documentation, and other artifacts. We consider + these all "code reviews" regardless of whether the final artefact is "code" + in a technical sense. +- Encourage broad participation and contribution to the community through code + reviews. +- Ensure code reviews are inclusive, respectful, and welcoming. +- Have clear, discoverable, and mechanically enforced (where possible) rules + for who _can_, who _should_, and who _must_ review any particular change. + +## Background + +General code review: + +- Chapter 9 "Code Review" in + _[Software Engineering at Google](https://www.amazon.com/Software-Engineering-Google-Lessons-Programming/dp/1492082791)_ +- Chapter 21 "Collaborative Construction" in + _[Code Complete: A Practical Handbook of Software Construction](https://www.amazon.com/Code-Complete-Practical-Handbook-Construction/dp/0735619670/)_ +- [Respectful Code Reviews (Chromium)](https://chromium.googlesource.com/chromium/src/+/master/docs/cr_respect.md) +- [Compassionate--Yet Candid--Code Reviews (video)](https://youtu.be/Ea8EiIPZvh0) +- [The Standard of Code Review](https://google.github.io/eng-practices/review/reviewer/standard.html) +- [How We Do Code Review](https://devblogs.microsoft.com/appcenter/how-the-visual-studio-mobile-center-team-does-code-review/) +- [Code Reviewing in the Trenches: Understanding Challenges, Best Practices and Tool Needs](https://www.microsoft.com/en-us/research/wp-content/uploads/2016/05/MS-Code-Review-Tech-Report-MSR-TR-2016-27.pdf) +- [The Importance of Code Reviews](https://www.sitepoint.com/the-importance-of-code-reviews/) +- [10 Reasons Why Code Reviews Make Better Code and Better Teams](https://simpleprogrammer.com/why-code-reviews-make-better-code-teams/) +- [Wikipedia article on code review](https://en.wikipedia.org/wiki/Code_review) +- [Expectations, Outcomes, and Challenges of Modern Code Review](https://sback.it/publications/icse2013.pdf) + +Specific GitHub tooling: + +- [About code owners](https://help.github.com/en/github/creating-cloning-and-archiving-repositories/about-code-owners) +- [Reviewing changes in pull requests](https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/reviewing-changes-in-pull-requests) + +## Proposal + +Add a [code review guide](/docs/project/code_review.md) to the project, and +reference it from our contributing guide and pull request workflow +documentation. + +Also create initial `CODEOWNERS` files in both this repository and the toolchain +repository based on current review activity and team roles. These are really +only suggested as an initial guess and should likely be iterated frequently as +more people join and begin contributing. The `carbon-lang` repository file is +included directly, and carbon-language/carbon-toolchain#1 updates the +`carbon-toolchain` repository. + +## Alternatives considered + +### Post-commit review + +Some projects, such as LLVM, use _post-commit_ review for some changes. This has +both pros and cons. + +Pros: + +- Enables sustaining velocity in the face of high latency in code review. + - Little need to rebase or resolve merge conflicts due to immediately + landing patches even while under review. + - Cross-developer dependencies don't create challenges. +- Optimizes commit velocity amongst a small group of developers with extensive + experience working together and minimal review comments on others' code. + +Cons: + +- Does not meaningfully scale beyond small group of developers with + pre-existing shared understanding of desired form of code. + - Relies on relative infrequency of needing code review comments. + - Developers largely need to be well aligned and reliably writing code + others would already approve. +- Disincentivizes code review relative to writing code. + - This effect is strong enough to create a significant fraction of commits + that simply see no review in the LLVM community. +- Creates significant barriers for new contributors. + - In practice, existing contributors are much more likely to be able to + accurately create post-commit review passing changes than new + contributors. + - Because of this, new contributors will have a very difficult time + joining the community. + - This has specifically been cited by people joining the LLVM community. + +### Skipping review when no functionality is changed (NFC commits) + +Another practice popular in the LLVM community is to skip pre-commit review for +changes for which "no functionality changes" or NFC commits. Common examples are +reformatting or basic code cleanup. The idea is that these are exceedingly lower +risk compared changes to functionality. + +Pros: + +- Can avoid waiting for a review on trivial cleanups and refactorings. + - May be especially useful as they tend to be merge conflict prone and + likely to be lead-ups to changes sent out for review. + - The advantage is lessened when using stacked reviews to parallelize + them. +- Avoid spending reviewer time on more trivial changes. + - Unclear how much time this is as NFC changes are typically relatively + fast to review due to their nature. Deciding "did it change behavior?" + is easier than deciding "given that it changes behavior, is the change + good?". + +Cons: + +- No concrete and objective rubric for whether or not a change is NFC, and + whether it perhaps is significant enough to still warrant review. + - Debating this adds a new cost to the entire process. + - In some cases, deciding whether a change is NFC roughly requires a code + review, making it pointless to skip the code review. + - A specific utility of code review is to discover when something + _unexpected_ happens. The fact that the author _believes_ a change is + NFC doesn't address this utility as the unexpected thing may be the + functionality changed. +- Loses the knowledge sharing benefit of code review for cleanups and + refactorings. + - Especially unfortunate as these are exactly the kinds of changes often + recommend for people starting to get familiar with a project. +- Fails to ensure consistency or ease of understanding of the code. + - Despite not changing functionality, a change may decrease how easily + understood the code is or may move it to be inconsistent with the wider + codebase. + - Avoiding these two things are some of the primary goals of code review.