steakhal wrote: > Sounds reasonable. > > I feel a bit guilty that I wasn't able to predict this regression when I > reviewed the reverted commit as "LGTM, clean little patch", but I don't think > that I could've done better without investing drastically more effort into > the review.
You actually challenged it in https://github.com/llvm/llvm-project/pull/114835#issuecomment-2459660939. I don't think it was strictly on us. It's a difficult subject, and a tempting little patch. We had lacking test coverage to surface this too. And I personally don't have the hardware/infra to do full scale A/B testing. There is no easy way to test changes for real, pre or post merge. I also raised this at Sonar, because we feel we observe more RT regressions or quality issues year over year, which adds burden to moving to a new llvm release; making it really costly. Probably more costly than keeping some bots and contributors the right to verify changes pre-merge; but this is this definitely won't happen short/mid term (if ever). > By the way does this revert affect any of the commits that were related to > the reverted one? Great question. I was really tempted to revert the sibling patch of this one: b96c24b8613036749e7ba28f0c7a837115ae9f91 In the end, my current opinion is to keep it because I didn't find evidence that it harms. Another patch that comes in my mind is made by @ziqingluo-90 db38cc27bc61cf2d53bcac1203722853610aa073. I didn't check if this revert would make that code unnecessary, but in theory, they could coexist AFAIU. That said, #115917 also caused trouble from the same patch-stack, but I'd insist that the intention there is warranted. So if it still has some issues, we should just fix those instead reverting that. https://github.com/llvm/llvm-project/pull/157480 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
