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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:57
 public:
+  RefactoringResultCollector(DiagnosticsEngine &DE) : Diags(DE) {}
   void handleError(llvm::Error Err) override {
----------------
sammccall wrote:
> why inject the DE here (and handle mapping errors both in the caller and here) rather than always doing the mapping in the caller?
We don't have control of the Err caller for this class (the Error is passed via the `handleError` interface)...so we need the DE in this class to do the expanding.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60821