[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

[Aaron Puchert via Phabricator via cfe-commits]

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