[PATCH] D108268: [Modules] Change result of reading AST block to llvm::Error instead

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In D108268#2961547 <https://reviews.llvm.org/D108268#2961547>, @bnbarham wrote:

> Unless we also change `DiagnosticEngine` it doesn't look like this is a viable solution. The `PartialDiagnostic` can't be emitted straight to `Diags`, since there may already be a diagnostic in flight (see `Error(unsigned DiagID, ...)`). The args in `PartialDiagnostic` are currently protected and it seems weird to change that, but even if they weren't it sort of defeats the purpose of using `DiagnosticError` in the first place.
>
> Any other ideas?

This might be a stupid idea and a bridge too far but what if delayed diagnostic was storing `PartialDiagnostic` and not three strings? This looks like a better API but I haven't tried it myself and  concerned we might not have diag allocator in all required places.

Another idea is to replace DiagnosedError with something like ThreeStringError (please don't use this name). I can be wrong but I find it easier to understand an error representing diagnostics compared to a marker that diagnostic was emitted-or-scheduled earlier.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108268/new/

https://reviews.llvm.org/D108268