[PATCH] D153131: [clang analysis][thread-safety] Handle return-by-reference...

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

In D153131#4653456 <https://reviews.llvm.org/D153131#4653456>, @aaronpuchert wrote:

> In D153131#4653412 <https://reviews.llvm.org/D153131#4653412>, @courbet wrote:
>
>> I also had some push back internally on adding this to the existing flag. I'm going to add `-Wthread-safety-reference-return`, can we start by not temporarily including it in `-Wthread-safety-reference` so that we can see how much work it it to fix those warnings ?
>
> Can you elaborate on this? What's the reasoning? Here are two reasons for having it as part of `-Wthread-safety-reference` right from the beginning:
>
> - `-Wthread-safety-reference` is already separate from `-Wthread-safety-analysis` because passing a reference does not imply an access. If you have the warning you're arguably already opting into this, and I don't see much of a difference between passing via parameter versus passing by return.
> - Most users don't follow all reviews or read the release notes in detail and won't notice the new flag until it shows up in their build log. So we'd just lose time.
>
> Since warning messages always indicate the warning flag and thus make disabling it easy, I don't see an issue with enabling it right away as part of `-Wthread-safety-reference`.
>
> Lastly, this doesn't seem complicated enough to warrant extended beta testing.

We have a large number of users of `-Werror -Wthread-safety-analysis` internally. When we make the new warnings part of that flag we cannot integrate because we're breaking all these users. If we don't integrate we can't run the new analysis to see what we would need to fix.

Introducing a new flag allows us to:

- keep the current analysis running for users of `-Wthread-safety-analysis`.
- progressively add `-Wthread-safety-analysis-reference-return` to these users across the codebase, fixing them or disabling analysis as needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153131