krasimir added inline comments.
================
Comment at: clangd/ASTManager.cpp:105
+ std::string Diagnostics;
+ decltype(this->FixIts) LocalFixIts; // Temporary storage
+ for (ASTUnit::stored_diag_iterator D = Unit->stored_diag_begin(),
----------------
Why not typedef the type of the FixIts map?
================
Comment at: clangd/ASTManager.cpp:124
+ clangd::Diagnostic Diag = {R, getSeverity(D->getLevel()), D->getMessage()};
+ auto &FixIts = LocalFixIts[Diag];
+ for (const FixItHint &Fix : D->getFixIts()) {
----------------
I prefer this local variable to have a different name.
================
Comment at: clangd/ASTManager.cpp:134
+ std::lock_guard<std::mutex> Guard(RequestLock);
+ FixIts = std::move(LocalFixIts);
}
----------------
Consider the following situation:
1. worker thread is at line 130 just before the critical section with full info
about file A in LocalFixits;
main thread takes the lock at line 159 in onDocumentAdd(file B)
2. main thread finishes the critical section, calling CV.notify_one(), which
has no effect and releases RequestLock.
3. worker thread then gets RequestLock and enters the critical section at line
133, which outputs essentially stale diagnostics.
Arguably this seems OK, but maybe we want a separate mutex controlling access
to the FixIts, which also might be more logical.
I can't find a real issue with the code as-is, you can leave it like this, I'm
just wondering.
================
Comment at: clangd/clients/clangd-vscode/src/extension.ts:26
+ textEditor.edit(mutator => {
+ for (let edit of edits) {
+
mutator.replace(vscodelc.Protocol2Code.asRange(edit.range), edit.newText);
----------------
Consider using `const` instead of `let`.
https://reviews.llvm.org/D30498
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
