[PATCH] D36398: [clangd] Check if CompileCommand has changed on forceReparse.

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

Also missing tests :)



================
Comment at: clangd/ClangdUnitStore.cpp:45
+             .first;
+    Result.RemovedFile = nullptr;
+  } else if (!compileCommandsAreEqual(It->second->getCompileCommand(),
----------------
ilya-biryukov wrote:
> klimek wrote:
> > Just say RemovedFile = nullptr in the struct?
> I'd argue that having it in the code, rather than declaration makes it more readable. Besides, default ctor of shared_ptr gives us nullptr anyway, so we can also leave it as is and remove the assignments altogether.
> 
> I've moved `=nullptr` assignments out of the if/else bodies. Let me know if it still looks ugly.
Ah, I missed that it's a smart pointer; in that case, yes, remove the = nullptr.


================
Comment at: clangd/ClangdUnitStore.h:45-48
+  struct RecreateResult {
+    std::shared_ptr<CppFile> FileInCollection;
+    std::shared_ptr<CppFile> RemovedFile;
+  };
----------------
ilya-biryukov wrote:
> klimek wrote:
> > Not obvious to me what things in there mean.
> Added a comment. Hopefully makes sense now.
Better, thanks. Now, why does this need to be shared_ptr (as opposed to unique_ptr)? Don't we always only have one?


https://reviews.llvm.org/D36398