[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

Tue Oct 11 11:47:12 PDT 2022

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