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

[David Blaikie via llvm-commits]

dwblaikie wrote:

> > 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.
> 
> This is the key here IMO. If we had taken this to the forums, I suspect some of these classes of changes would have seen pushback, either in a more general form or from subprojects. Historically, we have been pretty averse to widespread changes like those, we favor doing these changes at the same time as the same code lines are being moved or changed for other reasons. To cite some examples: we've tried to change the camel case convention in one sweep, we've tried to perform clang-format in a similar fashion, these were all shut off.

My experience has been pretty different - naming conventions and and formatting have generally been disfavored, yes - but refactorings like the ones we're discussing haven't, in my experience, ever received much, if any, pushback. Some of that happening in broader refactorings, some more ad-hoc. Some of this NFC work has even included things like making APIs match so that migrations to standard features are easier - such as the migration from llvm::Optional to std::optional.
 
> > I think these patches do qualify as not requiring precommit review according to policy
> 
> That's another issue here, it's pretty difficult to tell. A handful of those are probably fine, but if we count how many such changes were pushed last year, the volume is a bit high: 623 commits with the "NFC" tag since Jan 1 2023. Considering this volume of commits is coming from a single author, it's pretty difficult as a group to evaluate what's going on, and the cost of bugs like this can be high.

What sort of costs do you have in mind? I think these changes have been, on average, lower cost than most (that these sort of changes are generally pretty safe) - even this one seems like it was picked up pretty quickly, blamed appropriately, etc.

> > These aren't formatting or whitespace-only changes, though. So I don't think this applies.
> 
> I understand your point if take the policy literally, but IMO the spirit of that guideline is to avoid polluting the git log and avoid some downstream churn. Maybe we should try to clarify if this is the intent or not.

Those are the motivations, yes, weighed against the value of such name/whitespace changes - I think in the case of these sort of API refactorings/simplifications, the cost/benefit tips in favor of the changes moreso than name/whitespace changes (especially since these sort of refactorings have value on a per-application basis moreso than naming/whitespace where the benefit is mostly gained by pervasive changes & so there's more need to get buy-in to make such pervasive changes) - that these improve readability in ways that are easier to justify than pervasive naming/whitespace changes.

Like I said - generally these sort of changes were sent for review, a lot, early on, and at some point the feedback was so infrequent that it didn't seem worthwhile anymore.



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