[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

awarzynski added a comment. In D84362#2365153 , @rjmccall wrote: > I may not have been clear. I'm not saying SourceLocation is a meaningful > concept in the driver. Likewise, I should be more careful with respect to how I refer to `SourceLocation` (a cl

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

rjmccall added a comment. I may not have been clear. I'm not saying SourceLocation is a meaningful concept in the driver. I'm saying that if you generalize the concept of "source location" to "location in the input", there is a clear analogue in the driver (namely, a position in the argument

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

awarzynski added a comment. Hi @rjmccall , thank you for your quick reply! > It sounds like what you want is a diagnostic library that's almost completely > abstracted over the kinds of entities that can be stored in a diagnostic, > including the definition of a source location. No :) We sugge

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

rjmccall added a comment. It sounds like what you want is a diagnostic library that's almost completely abstracted over the kinds of entities that can be stored in a diagnostic, including the definition of a source location. I don't think that's incompatible with this patch; there's no need to

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

awarzynski added a comment. Hi All, I've just noticed this patch and realised that it takes `DiagnosticBuilder` in the direction tangential to what we proposed in [1]. I've just restarted our efforts with regard to refactoring these APIs and feel that it would be good to coordinate any future

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG7e561b62d2f2: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic (authored by yaxunl). Herald added a subscriber: dexo

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

rjmccall accepted this revision. rjmccall added a comment. Okay. This LGTM if you feel that the operator forwarders is the better approach. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84362/new/ https://reviews.llvm.org/D84362 ___ cfe-com

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

yaxunl updated this revision to Diff 299044. yaxunl marked 2 inline comments as done. yaxunl added a comment. Add constructors to StreamingDiagnostic. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84362/new/ https://reviews.llvm.org/D84362 Files: clang/include/clang/AST/ASTContext.h

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

rjmccall added inline comments. Comment at: clang/include/clang/Basic/PartialDiagnostic.h:66 +return *this; + } + yaxunl wrote: > rjmccall wrote: > > Why are these template operators necessary? The LHS of the `<<` should > > convert to a base reference typ

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

yaxunl marked 4 inline comments as done. yaxunl added inline comments. Comment at: clang/include/clang/Basic/Diagnostic.h:1065 +/// +class StreamableDiagnosticBase { +public: rjmccall wrote: > I think I would prefer `StreamingDiagnostic` as the class name here. r

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

yaxunl updated this revision to Diff 298368. yaxunl marked an inline comment as done. yaxunl added a comment. Rename StreamableDiagnosticBase to StreamingDiagnostic. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84362/new/ https://reviews.llvm.org/D84362 Files: clang/include/clang/AS

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

rjmccall added a comment. Thanks, this looks a lot better. Comment at: clang/include/clang/Basic/Diagnostic.h:1065 +/// +class StreamableDiagnosticBase { +public: I think I would prefer `StreamingDiagnostic` as the class name here. Comment at

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

yaxunl marked an inline comment as done. yaxunl added inline comments. Comment at: clang/include/clang/Basic/PartialDiagnostic.h:51 + : DiagID(DiagID) { +Allocator = &Allocator_; + } tra wrote: > Is there a particular reason to move field initialization

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

tra added inline comments. Comment at: clang/include/clang/Basic/PartialDiagnostic.h:51 + : DiagID(DiagID) { +Allocator = &Allocator_; + } Is there a particular reason to move field initialization into the body here and in other constructors? CHANGES

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

yaxunl updated this revision to Diff 298125. yaxunl added a comment. revised by John's comments. Extracted common part of DiagnosticEngine and PartialDiagnostics as DiagnosticStorage. Make member functions of the base class of DiagnosticBuilder and ParticalDiagnostics non-virtual. CHANGES SI

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

rjmccall added inline comments. Comment at: clang/include/clang/Basic/Diagnostic.h:1086-1090 /// Note that many of these will be created as temporary objects (many call /// sites), so we want them to be small and we never want their address taken. /// This ensures that compile

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

rnk added subscribers: rjmccall, rnk. rnk added inline comments. Comment at: clang/include/clang/Basic/Diagnostic.h:1086-1090 /// Note that many of these will be created as temporary objects (many call /// sites), so we want them to be small and we never want their address take

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

tra accepted this revision. tra added a comment. In D84362#2283078 , @tra wrote: > It's possible. Unfortunately it's only triggered by our internal tool and > it's hard to create a public reproducer for it. I'll debug and try to fix it > on Monday. It a

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

tra added a comment. In D84362#2283065 , @yaxunl wrote: > I think this is probably a different issue. The issue reported in D84364 > was introduced by change in D84364 > . It's possible.

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

yaxunl added a comment. In D84362#2283045 , @tra wrote: > In D84362#2282992 , @yaxunl wrote: > >> The fix is for the change in D84364 . It >> has no effect on the change in this rev

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

tra added a comment. In D84362#2282992 , @yaxunl wrote: > The fix is for the change in D84364 . It has > no effect on the change in this review. Are you sure the issue you saw is due > to change in this review instead of

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

yaxunl added a comment. In D84362#2282965 , @tra wrote: > In D84362#2282890 , @yaxunl wrote: > >> I have a fix for the issue reported in D84364 >> . Would you like to try? Thanks. >

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

tra reopened this revision. tra added a comment. This revision is now accepted and ready to land. In D84362#2282890 , @yaxunl wrote: > I have a fix for the issue reported in D84364 > . Would you like to try? Thanks. I can

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

yaxunl added a comment. In D84362#2279845 , @tra wrote: > In D84362#2279688 , @tra wrote: > >> Apparently this patch triggers compiler crashes on some of our code. I'll >> try to create a reproducer, but it would b

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

tra added a comment. In D84362#2279688 , @tra wrote: > Apparently this patch triggers compiler crashes on some of our code. I'll try > to create a reproducer, but it would be great to revert the patch for now. It's likely the same issue as the one report

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

tra added a comment. Apparently this patch triggers compiler crashes on some of our code. I'll try to create a reproducer, but it would be great to revert the patch for now. *** SIGSEGV (@0x7f68892d8fe8), received by PID 8210 (TID 8225) on cpu 65; stack trace: *** PC: @ 0x7f686d8a169d

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

This revision was automatically updated to reflect the committed changes. Closed by commit rGee5519d32357: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic (authored by yaxunl). Herald added a project: clang. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

tra accepted this revision. tra added a comment. This revision is now accepted and ready to land. This is a very nice cleanup. Thank you, Sam. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84362/new/ https://reviews.llvm.org/D84362 ___ cfe-co

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

yaxunl updated this revision to Diff 292319. yaxunl retitled this revision from "[NFC] Add missing functions to PartialDiagnostic" to "[NFC] Refactor DiagnosticBuilder and PartialDiagnostic". yaxunl edited the summary of this revision. yaxunl added a comment. Revised by Artem's comments. Extracte