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

[Vedant Kumar via Phabricator via cfe-commits]

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).


Repository:
  rL LLVM

https://reviews.llvm.org/D36969