[PATCH] D30498: [clangd] Add support for FixIts.

[Krasimir Georgiev via Phabricator via cfe-commits]

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