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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1046
 def ThreadSafetyPrecise    : DiagGroup<"thread-safety-precise">;
 def ThreadSafetyReference  : DiagGroup<"thread-safety-reference">;
+def ThreadSafetyReturn     : DiagGroup<"thread-safety-return">;
----------------
courbet wrote:
> aaronpuchert wrote:
> > Why not under `-Wthread-safety-reference`, as it's return-by-reference that you're warning on? This seems too small for a separate flag to me.
> The main reason it so that we provide a soft transition period for users: If we put that in `-Wthread-safety-reference`, we'll start breaking compile for people who use `-Werror`, while a separate flag allows a transition period where people opt into the new feature.
Transition flags can end up resulting in more churn, assuming that we eventually want to put this under `-Wthread-safety-reference`, because then you have two categories of users:
* those that opted in will eventually have to remove the flag again, and
* those that didn't will get hard errors on updating the compiler at that point.
Of course you might argue that the latter case can be prevented by carefully reading the release notes, but we know how often that happens.

I'd argue that if you're using `-Wthread-safety-reference`, you're already opting into warnings on escaping references, and not warning on `return` is a false negative.

A separate flag would make sense to me if we want to keep it, for example because this produces a substantial amount of false positives under some circumstances. Did you try this on a larger code base that's using the annotations? I could try it on our code, and maybe we can get some Googler to test it on theirs, which is also heavily using Thread Safety Analysis. (Though I'm not sure whether they use `-Wthread-safety-reference`.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153131