Page MenuHomePhabricator

[clangd] Emit better error messages when rename fails.
ClosedPublic

Authored by hokein on Apr 17 2019, 6:24 AM.

Details

Summary

Currently we emit an unfriendly "clang diagnostic" message when rename fails. This
patch makes clangd to emit a detailed diagnostic message.

Diff Detail

Event Timeline

hokein created this revision.Apr 17 2019, 6:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2019, 6:24 AM
sammccall added inline comments.Apr 18 2019, 1:00 AM
clang-tools-extra/clangd/ClangdServer.cpp
57 ↗(On Diff #195553)

why inject the DE here (and handle mapping errors both in the caller and here) rather than always doing the mapping in the caller?

60 ↗(On Diff #195553)

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?

315 ↗(On Diff #195553)

can we avoid this duplication by giving a helper function more responsibilities?
e.g. llvm::Error expandDiagnostics(llvm::Error, DiagnosticsEngine&)?

hokein updated this revision to Diff 195701.Apr 18 2019, 2:51 AM
hokein marked 4 inline comments as done.

Address comments.

hokein added inline comments.Apr 18 2019, 2:52 AM
clang-tools-extra/clangd/ClangdServer.cpp
57 ↗(On Diff #195553)

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.

315 ↗(On Diff #195553)

SG!

sammccall accepted this revision.Apr 18 2019, 3:22 AM

LG, thanks!

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

clang-tools-extra/clangd/ClangdServer.cpp
57 ↗(On Diff #195553)

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

This revision is now accepted and ready to land.Apr 18 2019, 3:22 AM
hokein updated this revision to Diff 195715.Apr 18 2019, 4:28 AM
hokein marked an inline comment as done.

Simplify the code further.

This revision was automatically updated to reflect the committed changes.