Page MenuHomePhabricator

[Basic] Add a DiagnosticError llvm::ErrorInfo subclass
ClosedPublic

Authored by arphaman on Aug 21 2017, 8:54 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.Aug 21 2017, 8:54 AM
arphaman edited the summary of this revision. (Show Details)Aug 21 2017, 8:57 AM
vsk edited edge metadata.Aug 21 2017, 11:35 AM

Would it be more convenient to extend ErrorInfo instead of creating a new diagnostic wrapper? E.g one benefit of passing around Expected<T>'s is that there's some assurance the diagnostics will be reported.

In D36969#847803, @vsk wrote:

Would it be more convenient to extend ErrorInfo instead of creating a new diagnostic wrapper? E.g one benefit of passing around Expected<T>'s is that there's some assurance the diagnostics will be reported.

Possibly, but one issue I found with ErrorInfo is that it gets harder to report diagnostics. E.g. with DiagnosticOr you will be able to write:

DiagnosticOr<...> function(RefactoringOperation &Op) {
  return Op.Diag(Loc, err::something) << "invalid";
}

But with Expected you'll have to use something like:

Expected<...> function(RefactoringOperation &Op) {
  return llvm::make_error<DiagnosticError>(Op.Diag(Loc, err::something) << "invalid");
}

I don't think that the assurance about reporting these diagnostics is that important for my case. They will be consumed at most in 3 places (clang-refactor, clangd, and libclang) and clangd and it will obvious to the user if errors aren't getting reported.

In D36969#847803, @vsk wrote:

Would it be more convenient to extend ErrorInfo instead of creating a new diagnostic wrapper? E.g one benefit of passing around Expected<T>'s is that there's some assurance the diagnostics will be reported.

Possibly, but one issue I found with ErrorInfo is that it gets harder to report diagnostics. E.g. with DiagnosticOr you will be able to write:

DiagnosticOr<...> function(RefactoringOperation &Op) {
  return Op.Diag(Loc, err::something) << "invalid";
}

But with Expected you'll have to use something like:

Expected<...> function(RefactoringOperation &Op) {
  return llvm::make_error<DiagnosticError>(Op.Diag(Loc, err::something) << "invalid");
}

I don't think that the assurance about reporting these diagnostics is that important for my case. They will be consumed at most in 3 places (clang-refactor, clangd, and libclang) and clangd and it will obvious to the user if errors aren't getting reported.

Although I guess I could always change the approach completely and use variadic templates instead of <<, e.g.:

template<typename... T>
Error RefactoringOperation::error(SourceLocation Loc, unsigned DiagId, const T &... Arguments) {
  return llvm::make_error<DiagnosticError>(...);
}

DiagnosticOr<...> function(RefactoringOperation &Op) {
   return Op.error(Loc, diag::err_something, "invalid");
}

I think I'll try that instead.

arphaman updated this revision to Diff 112169.Aug 22 2017, 6:45 AM
arphaman retitled this revision from [Basic] Add a DiagnosticOr type to [Basic] Add a DiagnosticError llvm::ErrorInfo subclass.
arphaman edited the summary of this revision. (Show Details)

Use ErrorInfo subclass as suggested by Vedant.

vsk added a comment.Aug 22 2017, 10:43 AM

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

unittests/Basic/DiagnosticTest.cpp
81 ↗(On Diff #112169)

It might be useful to add an Error-returning wrapper around PDA, e.g:
static Error PartialDiagnosticAt::get(...)

90 ↗(On Diff #112169)

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

arphaman marked 2 inline comments as done.Aug 23 2017, 3:31 AM
In D36969#848906, @vsk wrote:

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

Makes sense, I've added two similar helper functions in the updated patch.

arphaman updated this revision to Diff 112327.Aug 23 2017, 3:32 AM

Add helper functions as suggested by Vedant.

vsk accepted this revision.Aug 23 2017, 9:36 AM

LGTM!

This revision is now accepted and ready to land.Aug 23 2017, 9:36 AM
This revision was automatically updated to reflect the committed changes.