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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
