bwendling wrote: > > That approval wasn't rescinded > > I don't think you can reasonably expect a year old approval on a completely > different patch to carry over, regardless of whether it was explicitly > rescinded (which is an exotic operation on GitHub that few people even know > how to perform). I know that one of the reviewers did not intend their > approval to carry over (because they specifically complained about the PR > reuse going on here) and the other has not even looked at this PR since it > pivoted in a different direction (or at least they have given no indication > of having looked at it).
I chose to keep the changes in this PR because it kept context. And the reviewers who gave previous approvals did comment on recent changes. > > and the new implementation was reviewed by many people, including you, in > > detail. > > My review literally said: > > > Didn't get far, but a few initial comments. > > And I don't see anyone else who has done significant review on this either. > > Which is not surprising, given that this PR was not in a reviewable state > until a week ago, when the test coverage was fixed. The main point of a review is to check the algorithms, code fit, etc. The testcases are obviously important, but any changes to them would be separate from the main code review. The change is large, so I meant for people to start their reviews while I worked on the testcases. Once they were finished, no one had done any further comments, and so I begged for a week for people to comment *anything* on the PR. The very least you could have done is add a short note saying that you'll get to the review soon, or even an emoji, especially because you're an active reviewer. If there is literally zero feedback within a reasonable amount of time, there are only a few inferences one can make: the reviewer has no other comments and is fine with the current approvals, or the reviewer has lost interest. > > I TOLD YOU THAT I WOULDN'T BE BEST TO COMBINE THE TWO IN THE INITIAL > > IMPLEMENTATION! However, you and Nick both insisted that it be done this > > way. There's literally no reason for me to split the two up, because the > > renaming is NFC. Please don't move goalposts like this. > > I don't think I'm moving goalposts here? I already said back then: > > > Doing a CallBrPrepare -> InlineAsmPrepare as an NFC first would be good > > though. > > I guess I should have not phrased that as a suggestion? I must have missed that. My apologies. https://github.com/llvm/llvm-project/pull/92040 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
