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

[Ben Barham via Phabricator via cfe-commits]

bnbarham added a subscriber: yaxunl.
bnbarham added a comment.

> ! In D108268#2961735 <https://reviews.llvm.org/D108268#2961735>, @vsapsai wrote:
>  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.

It seems like it should be possible to not have the "only one in-flight diagnostic" restriction at all. Removing that would be nice since 1. we could just use DiagnosticError and 2. there would be no need to check for an existing diagnostic.

@yaxunl I see you did a bunch of work extracting out the DiagnosticStorage and StreamingDiagnostic classes. Any thoughts on this? I feel like I'm missing something obvious here.

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

There's only a few places that call `SetDelayedDiagnostic`, but none of them would have an allocator (ie. `DiagnosticIDs` and `ContentCache`). I may fallback to this instead (could just call it `ASTReadError`/something similar).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108268