ilya-biryukov added inline comments.
================
Comment at: clangd/ClangdLSPServer.cpp:495
+llvm::Optional<std::string> ClangdLSPServer::getDocument(PathRef File) {
+ llvm::Optional<std::string> Contents = DraftMgr.getDraft(File);
+ if (!Contents)
----------------
This function is equivalent to `return DraftMgr.getDraft()`. Remove and replace
all usages with `DraftMgr.getDraft()`?
================
Comment at: clangd/ClangdServer.cpp:154
llvm::Expected<InputsAndPreamble> IP) {
assert(IP && "error when trying to read preamble for codeComplete");
auto PreambleData = IP->Preamble;
----------------
Please replace this assert with:
```
if (!IP)
return CB(IP.takeError());
```
(Otherwise the assert will fire when completion is called for non-tracked
files, we really want this to be an error instead)
================
Comment at: clangd/ClangdServer.h:121
/// compilation database even if they were cached in previous invocations.
- void addDocument(PathRef File, StringRef Contents,
+ void addDocument(PathRef File, std::string Contents,
WantDiagnostics WD = WantDiagnostics::Auto,
----------------
Let's keep it a `StringRef`. It means an extra copy for now, `DraftMgr` return
`StringRef`s instead if we remove mutexes from its interface.
================
Comment at: clangd/ClangdServer.h:225
+ /// Document contents with a version of this content.
+ struct VersionedContents {
+ DocVersion Version;
----------------
We don't use `VersionedContents` anywhere. Maybe remove this struct?
================
Comment at: clangd/ClangdServer.h:236
FileSystemProvider &FSProvider;
- DraftStore DraftMgr;
+ llvm::StringMap<DocVersion> InternalVersion;
// The index used to look up symbols. This could be:
----------------
This is probably a good place for a small comment:
```
/// Used to synchronize diagnostic responses for added and removed files.
```
================
Comment at: clangd/DraftStore.h:36
+ void
+ forEachActiveDraft(std::function<void(PathRef, StringRef)> Callback) const;
----------------
Let's avoid calling `Callback` while holding a lock, it is very easy to come up
with a code that deadlocks.
I suggest either keeping the `getActiveFiles()` function or removing the mutex
and the locks. IIRC, the `DraftStore` is never actually used concurrently, so
removing mutexes should be no-op change.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44408
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
