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

Tue Oct 11 10:59:17 PDT 2022

hans added a comment.

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.

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.

Since the bug that Gulfem reported is fixed, relanding sounds good to me.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129755/new/

https://reviews.llvm.org/D129755