[PATCH] D156445: [BPF] Avoid repeating MI->getOperand(NumDefs) x3

Eduard Zingerman via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 2 09:33:04 PDT 2023


eddyz87 added a comment.

In D156445#4554360 <https://reviews.llvm.org/D156445#4554360>, @tamird wrote:

> In D156445#4554232 <https://reviews.llvm.org/D156445#4554232>, @eddyz87 wrote:
>
>> In my opinion, whether or not to repeat `MI->getOperand(NumDefs)` is a matter of personal preference in this case. Current code is ok and this change does not change any related functionality. I suggest to withheld from this change to avoid cluttering commit history.
>
> I seems obvious that readability of code-at-HEAD should be of a higher priority than the readability of the commit history; the extant code will be read many more times than the the commit history for the same piece of code.

I disagree that readability of these 2 lines had improved. I see this commit is an unnecessary code shuffling.

> The description of this change doesn't claim that there is a change in behavior here; is there documentation that requires all changes to have functional changes?
>
>> Also, I don't understand why you landed this change w/o due process.
>
> As I understand it, the process is not prescriptive about reviews being required. I see many changes (particularly in this area) that have gone in without review. In this case, I tried to follow "due process", but received no engagement whatsoever. Is filibustering an accepted practice?

Personally, I wait for reviewers engagements for all my changes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156445/new/

https://reviews.llvm.org/D156445



More information about the llvm-commits mailing list