[llvm] Add test for iterating over MDNode operands when they are empty (PR #80737)

David Blaikie via llvm-commits llvm-commits at lists.llvm.org
Tue Feb 6 13:08:44 PST 2024


dwblaikie wrote:

> Also wanted to take this opportunity to raise something related: I've noticed that the vast majority of your commits in the four months were NFC patches that were not previously posted for review. I'd really appreciate if subsequent patches like this went through the normal review process first, for a number of reasons:
> 
> The developer policy is pretty clear about [what can be committed without review](https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access):
> 
> > You are allowed to commit patches without approval which you think are obvious. This is clearly a subjective decision — we simply expect you to use good judgement. Examples include: fixing build breakage, reverting obviously broken patches, documentation/comment changes, any other minor changes.
> 
> Most of your patches would be candidates for the "other minor changes" category, which is a bit tricky to evaluate. As this bug proves, even the best intentioned NFC change can break things, and reviewers would have caught this.

I think these patches do qualify as not requiring precommit review according to policy - doesn't mean they can't be sent for review, might not need more care in the future (with or without precommit review) - but I don't think these commits were /against/ policy.

In general @kazutakahirata's commits, especially earlier on when they started on this sort of clang-tidy-like cleanups were precommit reviewed a lot - at some point the cost/benefit of that didn't seem worth it anymore, and most of the patches have been post-commit reviewed.

Personally, I'm OK with that - yes, some might require more care/fine tuning about how carefully to check the code/semantics of the replacement, but I think this is within the sort of thing that post-commit review/testing is suitable for.

If anything, maybe some place on the forums to post "does anyone see anything wrong with this general class of tidy/fixit/change (not each specific instance), if not, I'm going to apply this across the LLVM repo" for each type of change, perhaps.

> The policy also states that:
> 
> > Avoid committing formatting- or whitespace-only changes outside of code you plan to make subsequent changes to
> 
> In most cases, I believe you were not intending to make subsequent changes to that same piece of the codebase (based on the git log).

These aren't formatting or whitespace-only changes, though. So I don't think this applies.

https://github.com/llvm/llvm-project/pull/80737


More information about the llvm-commits mailing list