[PATCH] D60821: [clangd] Emit better error messages when rename fails.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:57
 public:
+  RefactoringResultCollector(DiagnosticsEngine &DE) : Diags(DE) {}
   void handleError(llvm::Error Err) override {
----------------
why inject the DE here (and handle mapping errors both in the caller and here) rather than always doing the mapping in the caller?


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:60
     assert(!Result.hasValue());
-    // FIXME: figure out a way to return better message for DiagnosticError.
-    // clangd uses llvm::toString to convert the Err to string, however, for
-    // DiagnosticError, only "clang diagnostic" will be generated.
-    Result = std::move(Err);
+    if (auto Diag = DiagnosticError::take(Err))
+      Result = toStringError(*Diag, Diags);
----------------
I'm confused about this, take() seems to return Optional<PartialDiagnosticAt>, but you're passing it as a DiagnosticError ... oh, DiagnosticError has a constructor from PartialDiagnosticAt.

This seems unneccesarily confusing, just pass the PartialDiagnostic?


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:315
+      auto Error = Rename.takeError();
+      if (auto Diag = DiagnosticError::take(Error)) {
+        llvm::cantFail(std::move(Error));
----------------
can we avoid this duplication by giving a helper function more responsibilities?
e.g. `llvm::Error expandDiagnostics(llvm::Error, DiagnosticsEngine&)`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60821/new/

https://reviews.llvm.org/D60821