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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D153131#4653564 <https://reviews.llvm.org/D153131#4653564>, @courbet wrote:

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

The proposal was to include it in `-Wthread-safety-reference`, not `-Wthread-safety-analysis`. See https://clang.llvm.org/docs/DiagnosticsReference.html#wthread-safety for the existing flags and their relations.

> If we don't integrate we can't run the new analysis to see what we would need to fix.

Can you not add `-Wno-error=thread-safety-reference-return` together with the integration? Or are there too many places adding it independently?

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

That is true, but these advantages seem to apply to a small number of users only (those aware of the new flag). If you integrate Clang trunk, it would be Ok if you leave it off by default for a couple of weeks, but turn it on before the next release.

I'm not generally against new flags, but this is more of a "gap closing" than a new feature, so an on-by-default (under `-Wthread-safety-reference`, not `-Wthread-safety-analysis`) warning should be the right choice. Changes that result in new warnings are not uncommon, and often we don't create a new flag for them at all. Here it's Ok due to the large number of warnings, but it fits too well into `-Wthread-safety-reference` to not be triggered by that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153131