[PATCH] D36969: [Basic] Add a DiagnosticError llvm::ErrorInfo subclass

Vedant Kumar via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Aug 22 10:43:17 PDT 2017

vsk added a comment.

Nice! I'd like to get your thoughts on two candidate ergonomic changes:

Comment at: unittests/Basic/DiagnosticTest.cpp:81
+  llvm::Expected<std::pair<int, int>> Value =
+      llvm::make_error<DiagnosticError>(PartialDiagnosticAt(
+          SourceLocation(), PartialDiagnostic(diag::err_cannot_open_file, Alloc)
It might be useful to add an Error-returning wrapper around PDA, e.g:
`static Error PartialDiagnosticAt::get(...)`

Comment at: unittests/Basic/DiagnosticTest.cpp:90
+    ErrDiag = std::move(Err.getDiagnostic());
+  });
+  EXPECT_EQ(ErrDiag.first, SourceLocation());
Creating a null diagnostic before taking an error may become cumbersome. An alternative is to add a static helper in DiagnosticError:

static ParitalDiagnosticAt &DiagnosticError::take(Error E) {
  PartialDiagnosticAt *PDA = nullptr;
  handleAllErrors(std::move(E), [&](DE) { PDA = &DE.getDiagnostic(); });
  assert(PDA && "Expected a DiagnosticError");
  return *PDA;

This is more ergonomic, but the downside is that you lose some flexibility (what happens if there are two kinds of errors?, etc).

Another alternative is to switch to a callback-driven style (though that might require a lot of refactoring).



More information about the cfe-commits mailing list