aaronpuchert marked 3 inline comments as done.
aaronpuchert added inline comments.
================
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);
----------------
aaron.ballman wrote:
> 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.
Ok, then let's leave it out for now.
================
Comment at: clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h:435
/// Used to implement lazy copy and rewriting strategies.
class Future : public SExpr {
public:
----------------
aaron.ballman wrote:
> 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.
Yes, it seems quite close. But my reading is that calling `result` is supposed
to force the evaluation, even when we might not be able to provide a name yet.
================
Comment at: clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h:629
+ else
+ SS << "<temporary>";
}
----------------
aaron.ballman wrote:
> aaronpuchert wrote:
> > Perhaps no warning will ever print this, but I'm not entirely sure.
> Should we assert here instead?
I played around a bit and it is actually possible to run into this in contrived
examples. I'll push some tests so that you can judge for yourself how we should
print this.
================
Comment at: clang/lib/Analysis/ThreadSafety.cpp:1538
FactSet FSet;
+ SmallVector<ConstructedObject, 2> ConstructedObjects;
LocalVariableMap::Context LVarCtx;
----------------
aaron.ballman wrote:
> 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?
I can't find an awful lot of documentation about this, but judging from the
name and a quick glance at the code I think that might be just what I'm looking
for. Thanks for the tip!
================
Comment at: clang/lib/Analysis/ThreadSafety.cpp:2422
+ case CFGElement::TemporaryDtor: {
+ CFGTemporaryDtor TD = BI.castAs<CFGTemporaryDtor>();
+
----------------
aaron.ballman wrote:
>
I was imitating lines 2405/2411, but `auto` is of course fine. (The code above
might actually predate C++11.)
================
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);
----------------
aaron.ballman wrote:
>
No doubt referring to
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return. I was
trying to emphasize the dual nature of `llvm::PointerUnion<const Expr *,
til::SExpr *>`, but I can certainly also drop the `else`.
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
