[PATCH] D36078: [XRay][compiler-rt] Remove use of std::mutex and std::shared_ptr from global scope.

[Keith via Phabricator via llvm-commits]

kpw accepted this revision.
kpw added a comment.
This revision is now accepted and ready to land.

Thanks for the clarification of intent Dean. LGTM, but I'd like to clear up one source of confusion with name collisions.



================
Comment at: lib/xray/xray_fdr_logging_impl.h:172
 // FDRLogging, and that we're going to clean it up when the thread exits.
-thread_local std::shared_ptr<BufferQueue> LocalBQ = nullptr;
-thread_local ThreadExitBufferCleanup Cleanup(LocalBQ, Buffer);
+thread_local std::shared_ptr<BufferQueue> *LocalBQ =
+    new std::shared_ptr<BufferQueue>();
----------------
Not a big deal, but earlier you declare "shared_ptr<T>* name" and here you have "shared_ptr<T> *name".

Not sure which is preferred in LLVM.


================
Comment at: lib/xray/xray_fdr_logging_impl.h:497
       writeEOBMetadata();
-      if (!releaseThreadLocalBuffer(LocalBQ.get()))
+      if (!releaseThreadLocalBuffer(*LocalBQ))
         return false;
----------------
Here LocalBQ is a shared_ptr& that shadows the thread local shared_ptr* of the same name. The shadowing is not new, but the types used to match.

I find this confusing. Can we rename the param LocalBQ to ParamBQ? Although I don't think dblakie's suggestion to pass \*\*LocalBQ from the call sites applies here too because \*\*LocalBQ is allowed to be nullptr if RecordPtr == nullptr or xray status is XRAY_LOG_INITIALIZING.


================
Comment at: lib/xray/xray_fdr_logging_impl.h:508
     writeEOBMetadata();
-    if (!releaseThreadLocalBuffer(LocalBQ.get()))
+    if (!releaseThreadLocalBuffer(*LocalBQ))
       return false;
----------------
Changing .get()s to *s for the smart ptr parameter is unnecessary and contributes to the confusion caused by the shadowing.


https://reviews.llvm.org/D36078