courbet added a comment.
Thanks.
================
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">;
----------------
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.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3805
+// Thread safety warnings on return
+def warn_guarded_return_by_reference : Warning<
----------------
aaronpuchert wrote:
> Or do you expect more warnings on return?
We could do pointers too, but arguably pointers and references are the same.
================
Comment at: clang/lib/Analysis/ThreadSafety.cpp:2159-2161
+ // If returning by reference, add the return value to the set to check on
+ // function exit.
+ if (RetVal->isLValue()) {
----------------
aaronpuchert wrote:
> Wouldn't it be more straightforward to check the actual return type? We have
> the `FunctionDecl` and could store it in `ThreadSafetyAnalyzer` instead of
> `CurrentMethod`.
Good point. I've also added better checking and diagnostics for `const`
(shared) vs `mutable` (exclusive) locks, with more tests.
================
Comment at: clang/lib/Analysis/ThreadSafety.cpp:2162
+ if (RetVal->isLValue()) {
+ Analyzer->ReturnValues.insert(RetVal);
+ }
----------------
aaronpuchert wrote:
> You're presumably collecting them because automatic destructor calls are
> after `return` in the CFG, right?
>
> If that's the case, can't we immediately check against the declared exit set?
> It should be known before we walk the CFG, unless I'm missing something.
> You're presumably collecting them because automatic destructor calls are
> after return in the CFG, right?
Exactly.
> If that's the case, can't we immediately check against the declared exit set?
> It should be known before we walk the CFG, unless I'm missing something.
Given how the code was written I was under the impression that we only knew the
entry set after walking the whole CFG (we're getting `ExpectedExitSet` after we
walk the CFG). But now I see that we're actually adressing the entry blok
beforehand. Thanks for the suggestion, this makes the code much simpler indeed
!
================
Comment at: clang/lib/Analysis/ThreadSafety.cpp:2165
+
+ VisitorBase::VisitReturnStmt(S);
+}
----------------
aaronpuchert wrote:
> Also wondering why we're doing this—no other visitor function seems to bother
> the `VisitorBase = ConstStmtVisitor<BuildLockset>`. Are these not just empty
> fallbacks?
The base code is hard to read because i't full of macros, but it looks like
it't probably empty indeed - done.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153131/new/
https://reviews.llvm.org/D153131
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
