[PATCH] D36397: [clangd] Fixed a data race.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdUnit.cpp:714
 
-void CppFile::cancelRebuilds() {
+void CppFile::cancelRebuild() { deferCancelRebuild().get(); }
+
----------------
klimek wrote:
> Somewhat orthogonal question - why CppFile? We do support other languages in Clangd, right? (C / Obj-C)
> I'd have expected this to be called ClangdUnit, especially given the file name.
Just wanted to give it a different name when the class changed semantics. Didn't rename the file yet, though, because was afraid that git would not detect renames and we'll lose history.
ClangdUnit used to provide implementations for all features (codeComplete, etc.), CppFile manages AST and Preamble., i.e. doing a different thing.
But I agree, CppFile is a probably a bad name, but I would still go with something not ending with Unit to not bring up parallels with ASTUnit, that provides a very different interface.

How about ClangdFile, ClangdFileAST? 


================
Comment at: clangd/ClangdUnit.cpp:735
+
+  std::shared_ptr<CppFile> That = shared_from_this();
+  return std::async(std::launch::deferred, [That, RequestRebuildCounter]() {
----------------
klimek wrote:
> Now I know why I don't like the shard_ptr, this is all pretty awkward.
Do you mean `shared_from_this()` specifically?


================
Comment at: clangd/ClangdUnit.cpp:738
+    std::unique_lock<std::mutex> Lock(That->Mutex);
+    CppFile *This = &*That;
+    This->RebuildCond.wait(Lock, [This, RequestRebuildCounter]() {
----------------
klimek wrote:
> Why do we need that instead of just calling the bound shared_ptr This?
This lambda is a deferred computation and might get executed on a different thread, so we want to keep a `shared_ptr` to our class around (`weak_ptr` would also work) to make sure this class does not get deleted before this lambda is called.
`This` local var is for capture in a lambda that is passed to `wait` in the code below (we could've captured `That`, but that would be an extra `shared_ptr` copy).



https://reviews.llvm.org/D36397