aaron.ballman added a comment.
Thank you for your patience on the review! I've taken a cursory look through
and I'm still thinking about the patch. I've not seen anything that's really
worrying. But this is pretty dense stuff and @delesley has way more experience
with TIL, so I'm hoping he might be able to take a peek as well.
================
Comment at: clang/docs/ThreadSafetyAnalysis.rst:935
+ // Same as constructors, but without tag types. (Requires C++17 copy
elision.)
+ static MutexLocker Lock(Mutex *mu) ACQUIRE(mu) NO_THREAD_SAFETY_ANALYSIS {
+ return MutexLocker(mu);
----------------
aaronpuchert wrote:
> The `NO_THREAD_SAFETY_ANALYSIS` here is due to a false positive warning
> because the scope is not destructed in the function body. Maybe we should
> postpone documentation until this is resolved.
I don't know that there's a lot of value to documenting it if we have to
disable thread safety analysis, so my instinct is to hold off on the
documentation for the moment.
================
Comment at: clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h:435
/// Used to implement lazy copy and rewriting strategies.
class Future : public SExpr {
public:
----------------
aaronpuchert wrote:
> By the way, I considered using this, but it didn't seem wholly appropriate.
> Our placeholder can't be lazily evaluated, we simply don't know initially.
> And later we do know and can just plug in the `VarDecl`.
Hmmm, naively, it seems like `FS_pending` is "we don't know initially" and
`FS_done` is "now we do know and can plugin in the `VarDecl`". But I agree it
seems a little different from lazy evaluation in that it's not a performance
optimization.
================
Comment at: clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h:629
+ else
+ SS << "<temporary>";
}
----------------
aaronpuchert wrote:
> Perhaps no warning will ever print this, but I'm not entirely sure.
Should we assert here instead?
================
Comment at: clang/lib/Analysis/ThreadSafety.cpp:1538
FactSet FSet;
+ SmallVector<ConstructedObject, 2> ConstructedObjects;
LocalVariableMap::Context LVarCtx;
----------------
aaronpuchert wrote:
> This is essentially an `std::map<const Expr *, til::LiteralPtr *>`, but since
> it only keeps objects between construction and assignment to a variable or
> temporary destruction, I thought that a small vector would be more
> appropriate. However, there is no guarantee that this doesn't grow in size
> significantly: temporaries with trivial destructors (which don't show up in
> the CFG) would never be removed from this list. That's unlikely for properly
> annotated scopes, but not impossible.
Would it make more sense to use a `SmallDenseMap` here?
================
Comment at: clang/lib/Analysis/ThreadSafety.cpp:2422
+ case CFGElement::TemporaryDtor: {
+ CFGTemporaryDtor TD = BI.castAs<CFGTemporaryDtor>();
+
----------------
================
Comment at: clang/lib/Analysis/ThreadSafetyCommon.cpp:339-342
+ if (const Expr *SelfArg = dyn_cast<const Expr *>(Ctx->SelfArg))
+ return translate(SelfArg, Ctx->Prev);
+ else
+ return cast<til::SExpr *>(Ctx->SelfArg);
----------------
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129755/new/
https://reviews.llvm.org/D129755
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
