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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to