aaron.ballman added a comment. In D129755#3850175 <https://reviews.llvm.org/D129755#3850175>, @hans wrote:
> In D129755#3849540 <https://reviews.llvm.org/D129755#3849540>, @aaron.ballman > wrote: > >>>> In D129755#3842744 <https://reviews.llvm.org/D129755#3842744>, @hans wrote: >>>> >>>>> >>>> >>>> I'll give you some time to reply before I reland, but I can't see a bug >>>> here. For the next time, as already pointed out, give the involved people >>>> some time to reply before you revert (unless it's a crash). We have to be >>>> able to continue working on the compiler without having Google revert >>>> changes all the time because we produce new warnings. >>> >>> We've seen reports here of various breakages from two different projects in >>> short time after your patch. The general policy is to revert to green, so I >>> think that was the right call. >> >> I don't think it was the wrong call per se, but it was an aggressive revert >> IMO. Chromium tends to revert changes very quickly and without discussion, >> and I'm hoping we can change that slightly to include more up-front >> discussion before a revert. It's one thing to revert when the patch author >> isn't responsive, but reverting immediately and without discussion when the >> commit message explicitly states there is a change in behavior isn't how I >> would expect the revert policy to be interpreted. > > In general I think us reverting fast is a good thing. Reverts and relands are > cheap, and reverting early prevents more people from being exposed to > breakages, which are expensive to investigate. Minimizing the amount time > where the source tree is in a bad state seems like a good thing to me, and > reverts also tend to become harder and more disruptive with time as more work > lands on top. In general, yes. Quick reverts for real problems are definitely appreciated (and our general policy to boot)! > I certainly don't want us to come across as aggressive though, and since for > this patch it turned out that the new false positives were expected, in > hindsight I probably shouldn't have been so quick to revert. Yup! I'm mostly just asking to raise awareness on the review before reverting in cases where it's not a build error (or other stop-the-world-this-is-obviously-wrong situation) and none of our community bots have gone red. Chromium is a downstream and we definitely want to make sure the ToT is usable for downstreams regardless of when they pull code, but downstreams should not have an expectation that any disturbance to their code base qualifies as something to revert without discussion. (We've had similar tension from in-tree downstreams as well, such as libc++ and lldb, so we're trying to find a happy medium between "instant revert of conforming changes without warning" and "leaving the ToT in a broken state too long and building up frustrating debt from that.") > Since the bug that Gulfem reported is fixed, relanding sounds good to me. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129755/new/ https://reviews.llvm.org/D129755 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
