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

Tue Oct 11 07:39:07 PDT 2022

aaron.ballman added a comment.

>> 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.

> Typically, new warnings are introduced behind new flags so that users can adopt them incrementally, though I realize that might be tricky in this instance. In any case, perhaps we should give other projects some time to assess the impact of this before relanding?

Other projects won't have the time to assess the impact of these changes because the changes were reverted (almost nobody applies one-off patches to try them out without there being something else driving that experiment). I think the changes should be re-landed so people can get us that feedback.

> This also seems like the kind of disruptive change we'd want to announce on Discourse, as discussed in Aaron's recent RFC <https://discourse.llvm.org/t/rfc-add-new-discourse-channel-for-potentially-breaking-disruptive-changes-for-clang/65251>. Probably should be mentioned in the "Potentially Breaking Changes" section of the release notes too.

+1!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129755