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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG, thanks!

I do think it can be further simplified, but if not then land as-is.



================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:57
 public:
+  RefactoringResultCollector(DiagnosticsEngine &DE) : Diags(DE) {}
   void handleError(llvm::Error Err) override {
----------------
hokein wrote:
> 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.
But we just store the err in Result, and then ClangdServer::rename does
```
if (!ResultCollector.Result.getValue())
      return CB(ResultCollector.Result->takeError());
```
can it wrap the takeError() in expandDiagnostics()?

it still has access to the diagnosticsengine I think


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60821