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

Felipe de Azevedo Piovezan via llvm-commits llvm-commits at lists.llvm.org
Mon Feb 5 15:45:28 PST 2024


felipepiovezan wrote:

> @kazutakahirata In hindsight your original patch would have benefitted from a pre-commit review. @rastogishubham Since there's more than one problem with it, I think it's best if you just revert [e851278](https://github.com/llvm/llvm-project/commit/e8512786fedbfa6ddba70ceddc29d7122173ba5e) but still update and land your test since it showcases a code path that previously wasn't covered.

Hi @kazutakahirata, it might be worth checking some of your other commits as well, I've seen at least one other that should probably be revert for the same reason: https://github.com/llvm/llvm-project/commit/7d269a484142459a1154ba81c68bf0c31f291fc8

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.

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).

Also, in general, the community is pretty sensible to large-scale refactors for a number of reasons that have been discussed in the past. While none of your patches are large-scale, their collective certainly is.

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


More information about the llvm-commits mailing list