nikic 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). > 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. > 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? https://github.com/llvm/llvm-project/pull/92040 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
