[PATCH] D36133: [clangd] Rewrote AST and Preamble management.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked 3 inline comments as done.
ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdServer.h:113
+  /// queue. The request will be run on a separate thread.
+  template <class Func, class... Args> void addToFront(Func &&F, Args &&... As);
+  /// Add a new request to run function \p F with args \p As to the end of the
----------------
bkramer wrote:
> Why the template? This is awkward now because it can only be called from ClangdServer.cpp
Storing `std::future`s  with `std::async(std::launch::deferred, ...` makes things easier than storing `std::function`, as that allows to move into the caller(`std::function` must be copyable).
But we want to store only properly created `std::future`s, so we create them on our own, hence the signature similar to `std::async`. 

I actually think that the signature itself is not that bad, but the fact that definition is only available in `.cpp` file is wrong. I've moved definintions to header.


================
Comment at: clangd/ClangdUnitStore.cpp:22
     return;
   OpenedFiles.erase(It);
 }
----------------
bkramer wrote:
> Not introduced in this commit, but this is equivalent to OpenedFiles.erase(File) without the find and check.
The code has changed a bit after initial submission. It returns removed element now and there seems to be no equivalent in `StringMap` for that.


https://reviews.llvm.org/D36133