[NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[yaxunl]

Summary

PartialDiagnostic misses some functions compared to DiagnosticBuilder.

This patch refactors DiagnosticBuilder and PartialDiagnostic, extracts
the common functionality so that the streaming << operators are
shared.

[tra]

FYI, the patch does not compile:

In file included from llvm/repo/clang/lib/Parse/ParseStmtAsm.cpp:13:
In file included from /llvm/repo/clang/include/clang/Parse/Parser.h:24:
llvm/repo/clang/include/clang/Sema/Sema.h:1833:10: error: use of overloaded operator '<<' is ambiguous (with operand types 'const clang::Sema::SemaDiagnosticBuilder' and 'clang::QualType')
      DB << T;
      ~~ ^  ~
llvm/repo/clang/include/clang/AST/Type.h:7144:28: note: candidate function [with DiagBuilderT = clang::Sema::SemaDiagnosticBuilder, $1 = nullptr]
inline const DiagBuilderT &operator<<(const DiagBuilderT &DB, QualType T) {
                           ^
llvm/repo/clang/include/clang/Sema/Sema.h:1508:41: note: candidate function [with T = clang::QualType]
    friend const SemaDiagnosticBuilder &operator<<(
                                        ^
llvm/repo/clang/include/clang/AST/TemplateBase.h:684:26: note: candidate function
const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,

[yaxunl]

fix build failure

[yaxunl]

ping. this is needed by https://reviews.llvm.org/D84364

[tra]

So, the idea here is to do some sort of duck-typing and allow DiagBuilder to work with both DiagnosticBuilder and PartialDiagnostic.

What bothers me is that unlike Diagnostic PartialDiagnostic seems to be commingling functionality of the builder with that of the diagnostic itself. I'm not sure if that's by design or just happened to be that way.

I think a better approach would be to refactor PartialDiagnostic and separate the builder functionality. That should make it possible to create a common diagnostic builder base class with Partial/Full diagnostic deriving their own builder, if needed.

That said, I'm not that familiar with the diags. Perhaps @rtrieu @aaron.ballman would have better ideas.

[aaron.ballman]

In D84362#2271585, @tra wrote:

So, the idea here is to do some sort of duck-typing and allow DiagBuilder to work with both DiagnosticBuilder and PartialDiagnostic.

What bothers me is that unlike Diagnostic PartialDiagnostic seems to be commingling functionality of the builder with that of the diagnostic itself. I'm not sure if that's by design or just happened to be that way.

I think a better approach would be to refactor PartialDiagnostic and separate the builder functionality. That should make it possible to create a common diagnostic builder base class with Partial/Full diagnostic deriving their own builder, if needed.

That said, I'm not that familiar with the diags. Perhaps @rtrieu @aaron.ballman would have better ideas.

I'm similarly a bit uncomfortable with adding the SFINAE magic to make this work instead of making a base class that will work for either kind of diagnostic builder. I'm adding @rsmith to see if he has opinions as well.

[yaxunl]

In D84362#2274315, @aaron.ballman wrote:

In D84362#2271585, @tra wrote:

So, the idea here is to do some sort of duck-typing and allow DiagBuilder to work with both DiagnosticBuilder and PartialDiagnostic.

What bothers me is that unlike Diagnostic PartialDiagnostic seems to be commingling functionality of the builder with that of the diagnostic itself. I'm not sure if that's by design or just happened to be that way.

I think a better approach would be to refactor PartialDiagnostic and separate the builder functionality. That should make it possible to create a common diagnostic builder base class with Partial/Full diagnostic deriving their own builder, if needed.

That said, I'm not that familiar with the diags. Perhaps @rtrieu @aaron.ballman would have better ideas.

I'm similarly a bit uncomfortable with adding the SFINAE magic to make this work instead of making a base class that will work for either kind of diagnostic builder. I'm adding @rsmith to see if he has opinions as well.

There are use patterns expecting PartialDiagnosticInst << X << Y to continue to be a PartialDiagnostic&, e.g.

PartialDiagnosticAt PDAt(SourceLoc, PartialDiagnosticInst << X << Y);

However if we derive PartialDiagnostic and DiagnosticBuilder from a base class DiagnosticBuilderBase which implements the << operators, PartialDiagnosticInst << X << Y will become a DiagnosticBuilderBase&, then we can no longer write the above code.

That's one reason I use templates to implement << operators.

Do we want to sacrifice this convenience?

[tra]

In D84362#2274790, @yaxunl wrote:

There are use patterns expecting PartialDiagnosticInst << X << Y to continue to be a PartialDiagnostic&, e.g.

PartialDiagnosticAt PDAt(SourceLoc, PartialDiagnosticInst << X << Y);

However if we derive PartialDiagnostic and DiagnosticBuilder from a base class DiagnosticBuilderBase which implements the << operators, PartialDiagnosticInst << X << Y will become a DiagnosticBuilderBase&, then we can no longer write the above code.

That's one reason I use templates to implement << operators.

Do we want to sacrifice this convenience?

I don't think we have to.
AFAICT, virtually all such patterns (and there are only 20-ish of them in total) are used with EmitFormatDiagnostic(S.PDiag()) which could be adapted to accept DiagnosticBuilderBase and Sema::PDiag() changed to return PartialDiagnosticBuilder with no loss of convenience.

[yaxunl]

In D84362#2274884, @tra wrote:

In D84362#2274790, @yaxunl wrote:

There are use patterns expecting PartialDiagnosticInst << X << Y to continue to be a PartialDiagnostic&, e.g.

PartialDiagnosticAt PDAt(SourceLoc, PartialDiagnosticInst << X << Y);

However if we derive PartialDiagnostic and DiagnosticBuilder from a base class DiagnosticBuilderBase which implements the << operators, PartialDiagnosticInst << X << Y will become a DiagnosticBuilderBase&, then we can no longer write the above code.

That's one reason I use templates to implement << operators.

Do we want to sacrifice this convenience?

I don't think we have to.
AFAICT, virtually all such patterns (and there are only 20-ish of them in total) are used with EmitFormatDiagnostic(S.PDiag()) which could be adapted to accept DiagnosticBuilderBase and Sema::PDiag() changed to return PartialDiagnosticBuilder with no loss of convenience.

I saw lots of usage of PartialDiagnosticAt, which is defined at

https://github.com/llvm/llvm-project/blob/8c3d0d6a5f5a2a521c4cbae7acbad82a49e2a92f/clang/include/clang/Basic/PartialDiagnostic.h#L417

It requres a PartialDiagnostic as the second argument.

A typical use is like

https://github.com/llvm/llvm-project/blob/69f98311ca42127df92527b6fc3be99841a15f12/clang/lib/Sema/AnalysisBasedWarnings.cpp#L1741

If we use a base class DiagnosticBuilderBase to implement << operators, S.PDiag(diag::warn_unlock_but_no_lock) << Kind << LockName becomes DiagnosticBuilderBase& and we can no longer write the above code.

There are ~100 uses like that.

[tra]

In D84362#2275029, @yaxunl wrote:

Perhaps we could inherit PartialDiagnostic from DiagnosticBuilder base class. This would probably be the least invasive approach as it would preserve existing uses while allowing reuse of common DiagnosticBuilder functionality. That said, it would muddle the meaning of PartialDiagnostic even more -- is it the diagnostic itself, or a thing that produces the diagnostic?

To think of it, we actually don't need the DiagnosticBuilderBase per se, but rather a limited subset of operations for implementing freestanding operator<<. So the base class for DiagnosticBuilder and PartialDiagnostic could be called StreamableDiagnosticBase and only provide those bits. This way the arrangement looks a bit less awkward. Both DiagnosticBuilder and PartialDiagnostic would inherit from it and would work with operator<<(StreamableDiagnosticBase). We may need to convert StreamableDiagnosticBase results back to the original type.

[yaxunl]

Revised by Artem's comments. Extracted the common code of DiagnosticBuilder and PartialDiagnostic
and removed redundant code.

[tra]

This is a very nice cleanup. Thank you, Sam.

[tra]

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  (unknown)  clang::Decl::getAvailability()
    @     0x7f6712fad13b       1376  FailureSignalHandler()
    @     0x7f696f1b69a0      34448  (unknown)
    @     0x7f688c278c9a        352  ShouldDiagnoseAvailabilityOfDecl()
    @     0x7f688c27897d        352  clang::Sema::DiagnoseAvailabilityOfDecl()
    @     0x7f688c8bd3c7       1584  clang::Sema::DiagnoseUseOfDecl()
    @     0x7f688c8cc1e8        672  clang::Sema::BuildDeclarationNameExpr()
    @     0x7f688c8c9e30        192  clang::Sema::BuildDeclarationNameExpr()
    @     0x7f688c8c2698       1280  clang::Sema::ActOnIdExpression()
    @     0x7f688e9b95db       9136  clang::Parser::ParseCastExpression()
    @     0x7f688e9b358b        128  clang::Parser::ParseCastExpression()
    @     0x7f688e9b9ebb       9136  clang::Parser::ParseCastExpression()
    @     0x7f688e9b358b        128  clang::Parser::ParseCastExpression()
    @     0x7f688e9b1998        112  clang::Parser::ParseAssignmentExpression()
    @     0x7f688e9b1832         64  clang::Parser::ParseExpression()
    @     0x7f688e9bd6f0       4064  clang::Parser::ParseParenExpression()
    @     0x7f688e9b6bb4       9136  clang::Parser::ParseCastExpression()
    @     0x7f688e9b358b        128  clang::Parser::ParseCastExpression()
    @     0x7f688e9b1998        112  clang::Parser::ParseAssignmentExpression()
    @     0x7f688e9c1285        288  clang::Parser::ParseExpressionList()
    @     0x7f688e9b4731       3088  clang::Parser::ParsePostfixExpressionSuffix()
    @     0x7f688e9bb92a       9136  clang::Parser::ParseCastExpression()
    @     0x7f688e9b358b        128  clang::Parser::ParseCastExpression()
    @     0x7f688e9b1998        112  clang::Parser::ParseAssignmentExpression()
    @     0x7f688e9b1832         64  clang::Parser::ParseExpression()
    @     0x7f688ea43040        240  clang::Parser::ParseReturnStatement()
    @     0x7f688ea3cef0       1264  clang::Parser::ParseStatementOrDeclarationAfterAttributes()
    @     0x7f688ea3c5cf        160  clang::Parser::ParseStatementOrDeclaration()
    @     0x7f688ea4490a       1264  clang::Parser::ParseCompoundStatementBody()
    @     0x7f688ea45df9        272  clang::Parser::ParseFunctionStatementBody()
    @     0x7f688ea6a7b4        960  clang::Parser::ParseFunctionDefinition()
    @     0x7f688e9600bd       2880  clang::Parser::ParseDeclGroup()
    @     0x7f688ea69634        368  clang::Parser::ParseDeclOrFunctionDefInternal()
    @     0x7f688ea68c20        672  clang::Parser::ParseDeclarationOrFunctionDefinition()
    @     0x7f688ea684ee        768  clang::Parser::ParseExternalDeclaration()
    @     0x7f688ea667a9        368  clang::Parser::ParseTopLevelDecl()
    @     0x7f688e93d2a2        240  clang::ParseAST()
    @     0x7f688fe079bd         80  clang::ASTFrontendAction::ExecuteAction()
    @     0x7f688fe073b8        144  clang::FrontendAction::Execute()
    @     0x7f688fd1bdd6        480  clang::ASTUnit::Parse()

[tra]

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 reported in D84364, but we probably triggered it independently.

[yaxunl]

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 be great to revert the patch for now.

It's likely the same issue as the one reported in D84364, but we probably triggered it independently.

I have a fix for the issue reported in D84364. Would you like to try? Thanks.

[tra]

In D84362#2282890, @yaxunl wrote:

I have a fix for the issue reported in D84364. Would you like to try? Thanks.

I can try it on the internal test that crashed with the patch. I've reopened this review and will pick up the diff once you update it.

[yaxunl]

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.

I can try it on the internal test that crashed with the patch. I've reopened this review and will pick up the diff once you update it.

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 change in D84364?

[tra]

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 change in D84364?

Pretty sure. We've bisected the failure specifically to commit ee5519d323571c4a9a7d92cb817023c9b95334cd, before D84364 landed.

[yaxunl]

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 review. Are you sure the issue you saw is due to change in this review instead of change in D84364?

Pretty sure. We've bisected the failure specifically to commit ee5519d323571c4a9a7d92cb817023c9b95334cd, before D84364 landed.

I think this is probably a different issue. The issue reported in D84364 was introduced by change in D84364.

[tra]

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

[tra]

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 appears the patch slightly increased stack use and caused a failure in code that was running with a limited stack space available.
AFAICT there are no issues with this patch per se and it's good to re-land, again.

[rnk]

clang/include/clang/Basic/Diagnostic.h

1217–1221

This comment documents that this object is intended to be small. Adding a vtable grows it by one pointer, and the virtual methods practically ensure that it will always be address-taken and will not be promoted from memory to registers. I think you need to reconsider the design of this patch. I will revert it for now. Please get approval from @rsmith or @rjmccall before adding a vtable to this class gain.

[rjmccall]

clang/include/clang/Basic/Diagnostic.h

1217–1221

I don't know that copying a DiagnosticBuilder is actually an important thing to support, but yes, this doesn't really seem like the right approach. The bigger problem is that it still leaves an enormous amount of code duplication between the active diagnostic being built in the DiagnosticsEngine and the diagnostic being built in the PartialDiagnostic — it's basically the same storage concept, duplicated in two different places! We should unify those two things, so that there's only one class implementing the storage of an active diagnostic. There can then be a common object of that class in DiagnosticsEngine, which DiagnosticBuilder then stores a reference to, while PartialDiagnostic has its own copy. And then it should be fairly straightforward to make the operators work with either.

Among other things, that might make it significantly more efficient to emit a diagnostic from a PartialDiagnostic — we might be able to just emit it from the internal storage instead of having to copy it into the engine, so that the engine's use of a common diagnostic object is just a storage optimization.

[yaxunl]

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.

[tra]

clang/include/clang/Basic/PartialDiagnostic.h

51

Is there a particular reason to move field initialization into the body here and in other constructors?

[yaxunl]

clang/include/clang/Basic/PartialDiagnostic.h

51

Allocator is now a member of the base class, therefore needs to be initialized directly. I could add ctor to the base class but I did not see too much benefit of doing that.

[rjmccall]

Thanks, this looks a lot better.

clang/include/clang/Basic/Diagnostic.h

1065

I think I would prefer StreamingDiagnostic as the class name here.

clang/include/clang/Basic/PartialDiagnostic.h

64

Why are these template operators necessary? The LHS of the << should convert to a base reference type just fine.

[yaxunl]

Rename StreamableDiagnosticBase to StreamingDiagnostic.

[yaxunl]

clang/include/clang/Basic/Diagnostic.h

1065

renamed

clang/include/clang/Basic/PartialDiagnostic.h

64

There are lots of usage patterns expecting the derived class after streaming, e.g.

void foo(PartialDiagnostic& PD);
foo(PD << X << Y);

[rjmccall]

clang/include/clang/Basic/PartialDiagnostic.h

64

I see. You could also just do this in the base operators by just making them templates, e.g.

template <class Diagnostic>
std::enable_if_t<std::is_base_of_v<StreamingDiagnostic, Diagnostic>, const Diagnostic &>
operator<<(const Diagnostic &D, ...) {
}

That way you'd never have these forwarding problems — but it would be more boilerplate for each operator, so maybe it's a wash.

[yaxunl]

Add constructors to StreamingDiagnostic.

[rjmccall]

Okay. This LGTM if you feel that the operator forwarders is the better approach.

[awarzynski]

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 efforts in this area. I also want to make sure that we have a solution that works for everyone.

Basically, this patch makes DiagnosticBuilder more integrated with libclangBasic by assuming that every diagnostic contains SourceLocations and FixItHints (see the definition of DiagnosticStorage). However, that's not the case for driver diagnostics (i.e. diagnostics issued in libclangDriver). In [1] we proposed to create thin abstraction layers above all diagnostic classes that do not depend on SourceLocation (or anything else from libclangBasic). This patch makes DiagnosticBuilder _more_ dependant on SourceLocation and in this respect refactors DiagnosticBuilder in a direction opposite to what we proposed in [1].

Since this patch changes the DiagnosticBuilder API quite significantly, I am not entirely sure how to proceed with refactoring it (we need to make it independent of SourceLocation and any related classes). Originally, DiagnosticBuilder was a standalone class, but now we will have to refactor the following classes on top of DiagnosticBuilder:

• DiagnosticStorage
• StreamingDiagnostic
• PartialDiagnostic

One idea that immediately comes to mind is to split DiagnosticStorage as follows:

struct DiagnosticStorage {
    // Storage that applies to all diagnostics
    // Original content, but without DiagRanges and FixItHints
};

struct LangDiagnosticStorage {
    // Storage that applies only to diagnostics that contain ranges and FixitHints - in practice these are language diagnostics
    SmallVector<CharSourceRange, 8> DiagRanges;
    SmallVector<FixItHint, 6> FixItHints;

    LangDiagnosticStorage() = default;
}

How does this look? Sadly it will clutter this API, but I can't see any way around it. One other extreme alternative is to:

• revert this patch
• create a thin abstraction layer for DiagnosticBuilder independent of SourceLocation (as per our RFC [1])
• re-apply this patch (with some necessary modifications)

I appreciate that it is important to keep this API as clean and as lean as possible. However,

• it's required by libclangDriver
• we want to make libclangDriver independent of Clang (so that it can be used by Flang without the dependency on Clang).

Further refactoring will be required to achieve this. We sent 2 RFCs [1] [2] earlier this year to cfe-dev to discuss this, so I hope that this won't come as a surprise.

Do you have any suggestions?

CC @tra @rjmccall @rsmith @yaxunl

Thank you,
Andrzej

[1] http://lists.llvm.org/pipermail/cfe-dev/2020-July/066393.html
[2] http://lists.llvm.org/pipermail/llvm-dev/2020-June/141994.html

[rjmccall]

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

Source locations, supplemental source ranges, and fix-its are all still meaningful concepts in the driver, it's just that a source location is, say, an offset into a particular argument rather than a location in the file. So the challenge for you is to come up with a way to abstract this that doesn't significantly make things worse for Clang.

[awarzynski]

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 suggested (and that's based on the feedback on cfe-dev) a more basic approach. We proposed an abstraction layer that does not require any location information, because that's irrelevant in the driver (more on this below). We did, intially, suggest re-using and re-factoring SourceLocation (and, more specifically, SourceManager). That direction was explicitely discouraged on cfe-dev [3].

Source locations, supplemental source ranges, and fix-its are all still meaningful concepts in the driver

When we studied the source code of libclangDriver that was not the case. The diagnostics in the driver [1] are only used to a very limited extent (compared to the frontend) and I've only seen them issued via [2]:

inline DiagnosticBuilder DiagnosticsEngine::Report(unsigned DiagID) {
  return Report(SourceLocation(), DiagID);
}

i.e. SourceLocation is not really used at all. Are we missing something here? Is there any place in the libclangDriver (or any tool that's implemented in terms of libclangdDriver) that actually requires SourceLocation?

We would like to keep the scope of refactoring libclangDriver (and its dependencies) to the required minimum. The solution that you suggested would mean refactoring an API that's not required by libclangDriver (please correct me if I'm missing something here). I'm concerned that any attempts to modify SourceLocation will be challenged on cfe-dev. It was already suggested that we shouldn't need it for what we want to achieve. And indeed, in our simplified prototype downstream we were able to avoid using it without affecting the driver diagnostics.

Another difficulty with your suggestion is testing. If we were to create some abstraction layer above SourceLocation - how would we define or test it? Again, AFAIK diagnostics in libclangDriver never contain any information with respect to source location. So how do we decide what's actually required? This would be easy if there was a reference example :)

All in all, I think that this patch will require us to broaden the refactoring in a way that otherwise wouldn't be required. And according to the feedback so far this will be detrimental to Clang. If we are still in disagreement then perhaps we should move this discussion to cfe-dev? I want to make sure that we are all aligned on the overall direction and that we don't diverge too much from what was already discussed on cfe-dev.

Thanks in advance for your feedback,
-Andrzej

[1]
https://github.com/llvm/llvm-project/blob/3c3071d5e7dd259022d4f5e879fdf9824b9e490c/clang/lib/Driver/OptionUtils.cpp#L26
https://github.com/llvm/llvm-project/blob/3c3071d5e7dd259022d4f5e879fdf9824b9e490c/clang/lib/Driver/Driver.cpp#L270
https://github.com/llvm/llvm-project/blob/3c3071d5e7dd259022d4f5e879fdf9824b9e490c/clang/lib/Driver/Driver.cpp#L275
https://github.com/llvm/llvm-project/blob/3c3071d5e7dd259022d4f5e879fdf9824b9e490c/clang/lib/Driver/Driver.cpp#L1145

[2] https://github.com/llvm/llvm-project/blob/3c3071d5e7dd259022d4f5e879fdf9824b9e490c/clang/include/clang/Basic/Diagnostic.h#L1482-L1484

[3] http://lists.llvm.org/pipermail/cfe-dev/2020-June/065766.html

[rjmccall]

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 list), and the only reason this isn't passed down to the driver and used in diagnostics is that we don't have the ability to express that today to the diagnostic engine. So instead of trying to extract out a part of the diagnostics engine that will work without any concept of source locations, you should be trying to parameterize the diagnostics engine so that it can work with an arbitrary external concept of source locations, and then the driver can use a different kind of source location than the main compiler and everything is fine.

[awarzynski]

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 class in Clang) and "source location" (location within a source file).

I see your point. Basically, you are suggesting that we improve the diagnostics for the driver first. And for that we'd need something like CommandLineLocation (a new class: location within the command invocation) that would be related to SourceLocation via a common base class, e.g. AbstractLocation (also a new class). That would indeed be a very nice thing to have.

However, this is not what we've been proposing. Our ultimate goal is a new Flang driver:

• implemented in terms of libclangDriver
• independent of Clang.

We shouldn't need to implement this extra functionality to achieve our goal. Personally I like the idea, but we need to stay focused on activities that are strictly needed for a new Flang driver. Not making things worse for Clang is an equally important goal. Improving things in Clang is something we are keen on, but are unlikely to have the bandwidth for.

It'll hard to create these thin layers above the diagnostic classes that do not depend on SourceLocation and SourceManager (and other classes that depend/require these). This is regardless of whether driver diagnostics are capable of tracking location (via e.g. CommandLineLocation) or not. The integration of diagnostics classes and "source location" related classes (specifically SourceLocation and SourceManager) is quite deep and this patch deepens that. IMO this patch makes things even harder for us. I'm starting to think that all this could be vastly simplified if libclangDriver had it's own diagnostics classes- a very simplified version of what's currently available in libclangBasic (again, because the driver doesn't need much). The improvements that you propose could be added at later time. Would this be acceptable? I think that the thin abstraction layer that we're discussing here would naturally emerge with time.

clang/include/clang/AST/ASTContext.h

private:
 
   /// The allocator used to create AST objects.
   ///
   /// AST objects are never destructed; rather, all memory associated with the
   /// AST objects will be released when the ASTContext itself is destroyed.
   mutable llvm::BumpPtrAllocator BumpAlloc;
 
   /// Allocator for partial diagnostics.
-  PartialDiagnostic::StorageAllocator DiagAllocator;
+  PartialDiagnostic::DiagStorageAllocator DiagAllocator;
 
   /// The current C++ ABI.
   std::unique_ptr<CXXABI> ABI;
   CXXABI *createCXXABI(const TargetInfo &T);
 
   /// The logical -> physical address space map.
   const LangASMap *AddrSpaceMap = nullptr;
 
public:
   /// AST nodes and type information.
   size_t getASTAllocatedMemory() const {
     return BumpAlloc.getTotalMemory();
   }
 
   /// Return the total memory used for various side tables.
   size_t getSideTableAllocatedMemory() const;
 
-  PartialDiagnostic::StorageAllocator &getDiagAllocator() {
+  PartialDiagnostic::DiagStorageAllocator &getDiagAllocator() {
     return DiagAllocator;
   }
 
   const TargetInfo &getTargetInfo() const { return *Target; }
   const TargetInfo *getAuxTargetInfo() const { return AuxTarget; }
 
   /// getIntTypeForBitwidth -
   /// sets integer QualTy according to specified details:

 
 private:
   /// All OMPTraitInfo objects live in this collection, one per
   /// `pragma omp [begin] declare variant` directive.
   SmallVector<std::unique_ptr<OMPTraitInfo>, 4> OMPTraitInfoVector;
 };
 
 /// Insertion operator for diagnostics.
-const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
+const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                       const ASTContext::SectionInfo &Section);
 
 /// Utility function for constructing a nullary selector.
 inline Selector GetNullarySelector(StringRef name, ASTContext &Ctx) {
   IdentifierInfo* II = &Ctx.Idents.get(name);
   return Ctx.Selectors.getSelector(0, &II);
 }
 
 /// Utility function for constructing an unary selector.

clang/include/clang/AST/Attr.h

return DuplicateArchitecture == Other.DuplicateArchitecture &&
            Tune == Other.Tune &&
            BranchProtection == Other.BranchProtection &&
            Features == Other.Features;
   }
 };
 
 #include "clang/AST/Attrs.inc"
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                              const Attr *At) {
   DB.AddTaggedVal(reinterpret_cast<intptr_t>(At),
                   DiagnosticsEngine::ak_attr);
   return DB;
 }
-
-inline const PartialDiagnostic &operator<<(const PartialDiagnostic &PD,
-                                           const Attr *At) {
-  PD.AddTaggedVal(reinterpret_cast<intptr_t>(At),
-                  DiagnosticsEngine::ak_attr);
-  return PD;
-}
 }  // end namespace clang
 
 #endif

clang/include/clang/AST/CanonicalType.h

 
 /// Represents a canonical, potentially-qualified type.
 using CanQualType = CanQual<Type>;
 
 inline CanQualType Type::getCanonicalTypeUnqualified() const {
   return CanQualType::CreateUnsafe(getCanonicalTypeInternal());
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                              CanQualType T) {
   DB << static_cast<QualType>(T);
   return DB;
 }
 
 //----------------------------------------------------------------------------//
 // Internal proxy classes used by canonical types
 //----------------------------------------------------------------------------//
 

clang/include/clang/AST/Decl.h

public:
   static EmptyDecl *CreateDeserialized(ASTContext &C, unsigned ID);
 
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
   static bool classofKind(Kind K) { return K == Empty; }
 };
 
 /// Insertion operator for diagnostics.  This allows sending NamedDecl's
 /// into a diagnostic with <<.
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
-                                           const NamedDecl* ND) {
-  DB.AddTaggedVal(reinterpret_cast<intptr_t>(ND),
-                  DiagnosticsEngine::ak_nameddecl);
-  return DB;
-}
-inline const PartialDiagnostic &operator<<(const PartialDiagnostic &PD,
-                                           const NamedDecl* ND) {
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &PD,
+                                             const NamedDecl *ND) {
   PD.AddTaggedVal(reinterpret_cast<intptr_t>(ND),
                   DiagnosticsEngine::ak_nameddecl);
   return PD;
 }
 
 template<typename decl_type>
 void Redeclarable<decl_type>::setPreviousDecl(decl_type *PrevDecl) {
   // Note: This routine is implemented here because we need both NamedDecl

clang/include/clang/AST/DeclCXX.h

public:
   void Profile(llvm::FoldingSetNodeID &ID) { Profile(ID, PartVal); }
 
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
   static bool classofKind(Kind K) { return K == Decl::MSGuid; }
 };
 
 /// Insertion operator for diagnostics.  This allows sending an AccessSpecifier
 /// into a diagnostic with <<.
-const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
-                                    AccessSpecifier AS);
-
-const PartialDiagnostic &operator<<(const PartialDiagnostic &DB,
+const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                       AccessSpecifier AS);
 
 } // namespace clang
 
 #endif // LLVM_CLANG_AST_DECLCXX_H

clang/include/clang/AST/DeclarationName.h

SourceLocation getEndLoc() const LLVM_READONLY {
     SourceLocation EndLoc = getEndLocPrivate();
     return EndLoc.isValid() ? EndLoc : getBeginLoc();
   }
 
 private:
   SourceLocation getEndLocPrivate() const;
 };
 
-/// Insertion operator for diagnostics.  This allows sending DeclarationName's
-/// into a diagnostic with <<.
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
-                                           DeclarationName N) {
-  DB.AddTaggedVal(N.getAsOpaqueInteger(),
-                  DiagnosticsEngine::ak_declarationname);
-  return DB;
-}
-
 /// Insertion operator for partial diagnostics.  This allows binding
 /// DeclarationName's into a partial diagnostic with <<.
-inline const PartialDiagnostic &operator<<(const PartialDiagnostic &PD,
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &PD,
                                              DeclarationName N) {
   PD.AddTaggedVal(N.getAsOpaqueInteger(),
                   DiagnosticsEngine::ak_declarationname);
   return PD;
 }
 
 raw_ostream &operator<<(raw_ostream &OS, DeclarationNameInfo DNInfo);
 
 } // namespace clang

clang/include/clang/AST/DependentDiagnostic.h

const PartialDiagnostic &getDiagnostic() const {
     return Diag;
   }
 
 private:
   friend class DeclContext::ddiag_iterator;
   friend class DependentStoredDeclsMap;
 
   DependentDiagnostic(const PartialDiagnostic &PDiag,
-                      PartialDiagnostic::Storage *Storage)
+                      DiagnosticStorage *Storage)
       : Diag(PDiag, Storage) {}
 
   static DependentDiagnostic *Create(ASTContext &Context,
                                      DeclContext *Parent,
                                      const PartialDiagnostic &PDiag);
 
   DependentDiagnostic *NextDiagnostic;
 
   PartialDiagnostic Diag;

clang/include/clang/AST/NestedNameSpecifier.h

public:
   /// buffer.
   std::pair<char *, unsigned> getBuffer() const {
     return std::make_pair(Buffer, BufferSize);
   }
 };
 
 /// Insertion operator for diagnostics.  This allows sending
 /// NestedNameSpecifiers into a diagnostic with <<.
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                              NestedNameSpecifier *NNS) {
   DB.AddTaggedVal(reinterpret_cast<intptr_t>(NNS),
                   DiagnosticsEngine::ak_nestednamespec);
   return DB;
 }
 
 } // namespace clang
 
 namespace llvm {

clang/include/clang/AST/TemplateBase.h

void initializeFrom(SourceLocation TemplateKWLoc,
                       TemplateArgumentLoc *OutArgArray,
                       TemplateArgumentDependence &Deps);
   void initializeFrom(SourceLocation TemplateKWLoc);
 
   void copyInto(const TemplateArgumentLoc *ArgArray,
                 TemplateArgumentListInfo &List) const;
 };
 
-const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
+const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                       const TemplateArgument &Arg);
 
 inline TemplateSpecializationType::iterator
     TemplateSpecializationType::end() const {
   return getArgs() + getNumArgs();
 }
 
 inline DependentTemplateSpecializationType::iterator
     DependentTemplateSpecializationType::end() const {

clang/include/clang/AST/TemplateName.h

public:
   /// Build a template name from a void pointer.
   static TemplateName getFromVoidPointer(void *Ptr) {
     return TemplateName(Ptr);
   }
 };
 
 /// Insertion operator for diagnostics.  This allows sending TemplateName's
 /// into a diagnostic with <<.
-const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
-                                    TemplateName N);
-const PartialDiagnostic &operator<<(const PartialDiagnostic &PD,
+const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                       TemplateName N);
 
 /// A structure for storing the information associated with a
 /// substituted template template parameter.
 class SubstTemplateTemplateParmStorage
   : public UncommonTemplateNameStorage, public llvm::FoldingSetNode {
   friend class ASTContext;
 
   TemplateTemplateParmDecl *Parameter;

clang/include/clang/AST/Type.h

 inline const Type *Type::getPointeeOrArrayElementType() const {
   const Type *type = this;
   if (type->isAnyPointerType())
     return type->getPointeeType().getTypePtr();
   else if (type->isArrayType())
     return type->getBaseElementTypeUnsafe();
   return type;
 }
-/// Insertion operator for diagnostics. This allows sending address spaces into
-/// a diagnostic with <<.
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
-                                           LangAS AS) {
-  DB.AddTaggedVal(static_cast<std::underlying_type_t<LangAS>>(AS),
-                  DiagnosticsEngine::ArgumentKind::ak_addrspace);
-  return DB;
-}
-
 /// Insertion operator for partial diagnostics. This allows sending adress
 /// spaces into a diagnostic with <<.
-inline const PartialDiagnostic &operator<<(const PartialDiagnostic &PD,
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &PD,
                                              LangAS AS) {
   PD.AddTaggedVal(static_cast<std::underlying_type_t<LangAS>>(AS),
                   DiagnosticsEngine::ArgumentKind::ak_addrspace);
   return PD;
 }
 
-/// Insertion operator for diagnostics. This allows sending Qualifiers into a
-/// diagnostic with <<.
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
-                                           Qualifiers Q) {
-  DB.AddTaggedVal(Q.getAsOpaqueValue(),
-                  DiagnosticsEngine::ArgumentKind::ak_qual);
-  return DB;
-}
-
 /// Insertion operator for partial diagnostics. This allows sending Qualifiers
 /// into a diagnostic with <<.
-inline const PartialDiagnostic &operator<<(const PartialDiagnostic &PD,
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &PD,
                                              Qualifiers Q) {
   PD.AddTaggedVal(Q.getAsOpaqueValue(),
                   DiagnosticsEngine::ArgumentKind::ak_qual);
   return PD;
 }
 
-/// Insertion operator for diagnostics.  This allows sending QualType's into a
-/// diagnostic with <<.
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
-                                           QualType T) {
-  DB.AddTaggedVal(reinterpret_cast<intptr_t>(T.getAsOpaquePtr()),
-                  DiagnosticsEngine::ak_qualtype);
-  return DB;
-}
-
 /// Insertion operator for partial diagnostics.  This allows sending QualType's
 /// into a diagnostic with <<.
-inline const PartialDiagnostic &operator<<(const PartialDiagnostic &PD,
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &PD,
                                              QualType T) {
   PD.AddTaggedVal(reinterpret_cast<intptr_t>(T.getAsOpaquePtr()),
                   DiagnosticsEngine::ak_qualtype);
   return PD;
 }
 
 // Helper class template that is used by Type::getAs to ensure that one does
 // not try to look through a qualified type to get to an array type.
 template <typename T>

clang/include/clang/Basic/Diagnostic.h

public:
   }
 
   static FixItHint CreateReplacement(SourceRange RemoveRange,
                                      StringRef Code) {
     return CreateReplacement(CharSourceRange::getTokenRange(RemoveRange), Code);
   }
 };
 
+struct DiagnosticStorage {
+  enum {
+    /// The maximum number of arguments we can hold. We
+    /// currently only support up to 10 arguments (%0-%9).
+    ///
+    /// A single diagnostic with more than that almost certainly has to
+    /// be simplified anyway.
+    MaxArguments = 10
+  };
+
+  /// The number of entries in Arguments.
+  unsigned char NumDiagArgs = 0;
+
+  /// Specifies for each argument whether it is in DiagArgumentsStr
+  /// or in DiagArguments.
+  unsigned char DiagArgumentsKind[MaxArguments];
+
+  /// The values for the various substitution positions.
+  ///
+  /// This is used when the argument is not an std::string. The specific value
+  /// is mangled into an intptr_t and the interpretation depends on exactly
+  /// what sort of argument kind it is.
+  intptr_t DiagArgumentsVal[MaxArguments];
+
+  /// The values for the various substitution positions that have
+  /// string arguments.
+  std::string DiagArgumentsStr[MaxArguments];
+
+  /// The list of ranges added to this diagnostic.
+  SmallVector<CharSourceRange, 8> DiagRanges;
+
+  /// If valid, provides a hint with some code to insert, remove, or
+  /// modify at a particular position.
+  SmallVector<FixItHint, 6> FixItHints;
+
+  DiagnosticStorage() = default;
+};
+
 /// Concrete class used by the front-end to report problems and issues.
 ///
 /// This massages the diagnostics (e.g. handling things like "report warnings
 /// as errors" and passes them off to the DiagnosticConsumer for reporting to
 /// the user. DiagnosticsEngine is tied to one translation unit and one
 /// SourceManager.
 class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
 public:
private:
   unsigned CurDiagID;
 
   enum {
     /// The maximum number of arguments we can hold.
     ///
     /// We currently only support up to 10 arguments (%0-%9).  A single
     /// diagnostic with more than that almost certainly has to be simplified
     /// anyway.
-    MaxArguments = 10,
+    MaxArguments = DiagnosticStorage::MaxArguments,
   };
 
-  /// The number of entries in Arguments.
-  signed char NumDiagArgs;
-
-  /// Specifies whether an argument is in DiagArgumentsStr or
-  /// in DiagArguments.
-  ///
-  /// This is an array of ArgumentKind::ArgumentKind enum values, one for each
-  /// argument.
-  unsigned char DiagArgumentsKind[MaxArguments];
-
-  /// Holds the values of each string argument for the current
-  /// diagnostic.
-  ///
-  /// This is only used when the corresponding ArgumentKind is ak_std_string.
-  std::string DiagArgumentsStr[MaxArguments];
-
-  /// The values for the various substitution positions.
-  ///
-  /// This is used when the argument is not an std::string.  The specific
-  /// value is mangled into an intptr_t and the interpretation depends on
-  /// exactly what sort of argument kind it is.
-  intptr_t DiagArgumentsVal[MaxArguments];
-
-  /// The list of ranges added to this diagnostic.
-  SmallVector<CharSourceRange, 8> DiagRanges;
-
-  /// If valid, provides a hint with some code to insert, remove,
-  /// or modify at a particular position.
-  SmallVector<FixItHint, 8> DiagFixItHints;
+  DiagnosticStorage DiagStorage;
 
   DiagnosticMapping makeUserMapping(diag::Severity Map, SourceLocation L) {
     bool isPragma = L.isValid();
     DiagnosticMapping Mapping =
         DiagnosticMapping::Make(Map, /*IsUser=*/true, isPragma);
 
     // If this is a pragma mapping, then set the diagnostic mapping flags so
     // that we override command line options.
public:
 
   /// Set to initial state of "no errors occurred".
   void reset() {
     NumErrors = Diag.TrapNumErrorsOccurred;
     NumUnrecoverableErrors = Diag.TrapNumUnrecoverableErrorsOccurred;
   }
 };
 
+/// The streaming interface shared between DiagnosticBuilder and
+/// PartialDiagnostic. This class is not intended to be constructed directly
+/// but only as base class of DiagnosticBuilder and PartialDiagnostic builder.
+///
+/// Any new type of argument accepted by DiagnosticBuilder and PartialDiagnostic
+/// should be implemented as a '<<' operator of StreamingDiagnostic, e.g.
+///
+/// const StreamingDiagnostic&
+/// operator<<(const StreamingDiagnostic&, NewArgType);
+///
+class StreamingDiagnostic {
+public:
+  /// An allocator for DiagnosticStorage objects, which uses a small cache to
+  /// objects, used to reduce malloc()/free() traffic for partial diagnostics.
+  class DiagStorageAllocator {
+    static const unsigned NumCached = 16;
+    DiagnosticStorage Cached[NumCached];
+    DiagnosticStorage *FreeList[NumCached];
+    unsigned NumFreeListEntries;
+
+  public:
+    DiagStorageAllocator();
+    ~DiagStorageAllocator();
+
+    /// Allocate new storage.
+    DiagnosticStorage *Allocate() {
+      if (NumFreeListEntries == 0)
+        return new DiagnosticStorage;
+
+      DiagnosticStorage *Result = FreeList[--NumFreeListEntries];
+      Result->NumDiagArgs = 0;
+      Result->DiagRanges.clear();
+      Result->FixItHints.clear();
+      return Result;
+    }
+
+    /// Free the given storage object.
+    void Deallocate(DiagnosticStorage *S) {
+      if (S >= Cached && S <= Cached + NumCached) {
+        FreeList[NumFreeListEntries++] = S;
+        return;
+      }
+
+      delete S;
+    }
+  };
+
+protected:
+  mutable DiagnosticStorage *DiagStorage = nullptr;
+
+  /// Allocator used to allocate storage for this diagnostic.
+  DiagStorageAllocator *Allocator = nullptr;
+
+public:
+  /// Retrieve storage for this particular diagnostic.
+  DiagnosticStorage *getStorage() const {
+    if (DiagStorage)
+      return DiagStorage;
+
+    assert(Allocator);
+    DiagStorage = Allocator->Allocate();
+    return DiagStorage;
+  }
+
+  void freeStorage() {
+    if (!DiagStorage)
+      return;
+
+    // The hot path for PartialDiagnostic is when we just used it to wrap an ID
+    // (typically so we have the flexibility of passing a more complex
+    // diagnostic into the callee, but that does not commonly occur).
+    //
+    // Split this out into a slow function for silly compilers (*cough*) which
+    // can't do decent partial inlining.
+    freeStorageSlow();
+  }
+
+  void freeStorageSlow() {
+    if (!Allocator)
+      return;
+    Allocator->Deallocate(DiagStorage);
+    DiagStorage = nullptr;
+  }
+
+  void AddTaggedVal(intptr_t V, DiagnosticsEngine::ArgumentKind Kind) const {
+    if (!DiagStorage)
+      DiagStorage = getStorage();
+
+    assert(DiagStorage->NumDiagArgs < DiagnosticStorage::MaxArguments &&
+           "Too many arguments to diagnostic!");
+    DiagStorage->DiagArgumentsKind[DiagStorage->NumDiagArgs] = Kind;
+    DiagStorage->DiagArgumentsVal[DiagStorage->NumDiagArgs++] = V;
+  }
+
+  void AddString(StringRef V) const {
+    if (!DiagStorage)
+      DiagStorage = getStorage();
+
+    assert(DiagStorage->NumDiagArgs < DiagnosticStorage::MaxArguments &&
+           "Too many arguments to diagnostic!");
+    DiagStorage->DiagArgumentsKind[DiagStorage->NumDiagArgs] =
+        DiagnosticsEngine::ak_std_string;
+    DiagStorage->DiagArgumentsStr[DiagStorage->NumDiagArgs++] = std::string(V);
+  }
+
+  void AddSourceRange(const CharSourceRange &R) const {
+    if (!DiagStorage)
+      DiagStorage = getStorage();
+
+    DiagStorage->DiagRanges.push_back(R);
+  }
+
+  void AddFixItHint(const FixItHint &Hint) const {
+    if (Hint.isNull())
+      return;
+
+    if (!DiagStorage)
+      DiagStorage = getStorage();
+
+    DiagStorage->FixItHints.push_back(Hint);
+  }
+
+  /// Conversion of StreamingDiagnostic to bool always returns \c true.
+  ///
+  /// This allows is to be used in boolean error contexts (where \c true is
+  /// used to indicate that an error has occurred), like:
+  /// \code
+  /// return Diag(...);
+  /// \endcode
+  operator bool() const { return true; }
+
+protected:
+  StreamingDiagnostic() = default;
+
+  /// Construct with an external storage not owned by itself. The allocator
+  /// is a null pointer in this case.
+  explicit StreamingDiagnostic(DiagnosticStorage *Storage)
+      : DiagStorage(Storage) {}
+
+  /// Construct with a storage allocator which will manage the storage. The
+  /// allocator is not a null pointer in this case.
+  explicit StreamingDiagnostic(DiagStorageAllocator &Alloc)
+      : Allocator(&Alloc) {}
+
+  StreamingDiagnostic(const StreamingDiagnostic &Diag) = default;
+  StreamingDiagnostic(StreamingDiagnostic &&Diag) = default;
+
+  ~StreamingDiagnostic() { freeStorage(); }
+};
+
 //===----------------------------------------------------------------------===//
 // DiagnosticBuilder
 //===----------------------------------------------------------------------===//
 
 /// A little helper class used to produce diagnostics.
 ///
 /// This is constructed by the DiagnosticsEngine::Report method, and
 /// allows insertion of extra information (arguments and source ranges) into
 /// the currently "in flight" diagnostic.  When the temporary for the builder
 /// is destroyed, the diagnostic is issued.
 ///
 /// 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 compilers with somewhat reasonable optimizers will promote
 /// the common fields to registers, eliminating increments of the NumArgs field,
 /// for example.
-class DiagnosticBuilder {
+class DiagnosticBuilder : public StreamingDiagnostic {
   friend class DiagnosticsEngine;
   friend class PartialDiagnostic;
 
   mutable DiagnosticsEngine *DiagObj = nullptr;
-  mutable unsigned NumArgs = 0;
 
   /// Status variable indicating if this diagnostic is still active.
   ///
   // NOTE: This field is redundant with DiagObj (IsActive iff (DiagObj == 0)),
   // but LLVM is not currently smart enough to eliminate the null check that
   // Emit() would end up with if we used that as our status variable.
   mutable bool IsActive = false;
 
   /// Flag indicating that this diagnostic is being emitted via a
   /// call to ForceEmit.
   mutable bool IsForceEmit = false;
 
   DiagnosticBuilder() = default;
 
   explicit DiagnosticBuilder(DiagnosticsEngine *diagObj)
-      : DiagObj(diagObj), IsActive(true) {
+      : StreamingDiagnostic(&diagObj->DiagStorage), DiagObj(diagObj),
+        IsActive(true) {
     assert(diagObj && "DiagnosticBuilder requires a valid DiagnosticsEngine!");
-    diagObj->DiagRanges.clear();
-    diagObj->DiagFixItHints.clear();
+    assert(DiagStorage &&
+           "DiagnosticBuilder requires a valid DiagnosticStorage!");
+    DiagStorage->NumDiagArgs = 0;
+    DiagStorage->DiagRanges.clear();
+    DiagStorage->FixItHints.clear();
   }
 
 protected:
-  void FlushCounts() {
-    DiagObj->NumDiagArgs = NumArgs;
-  }
-
   /// Clear out the current diagnostic.
   void Clear() const {
     DiagObj = nullptr;
     IsActive = false;
     IsForceEmit = false;
   }
 
   /// Determine whether this diagnostic is still active.
   bool isActive() const { return IsActive; }
 
   /// Force the diagnostic builder to emit the diagnostic now.
   ///
   /// Once this function has been called, the DiagnosticBuilder object
   /// should not be used again before it is destroyed.
   ///
   /// \returns true if a diagnostic was emitted, false if the
   /// diagnostic was suppressed.
   bool Emit() {
     // If this diagnostic is inactive, then its soul was stolen by the copy ctor
     // (or by a subclass, as in SemaDiagnosticBuilder).
     if (!isActive()) return false;
 
-    // When emitting diagnostics, we set the final argument count into
-    // the DiagnosticsEngine object.
-    FlushCounts();
-
     // Process the diagnostic.
     bool Result = DiagObj->EmitCurrentDiagnostic(IsForceEmit);
 
     // This diagnostic is dead.
     Clear();
 
     return Result;
   }
 
 public:
   /// Copy constructor.  When copied, this "takes" the diagnostic info from the
   /// input and neuters it.
   DiagnosticBuilder(const DiagnosticBuilder &D) {
     DiagObj = D.DiagObj;
+    DiagStorage = D.DiagStorage;
     IsActive = D.IsActive;
     IsForceEmit = D.IsForceEmit;
     D.Clear();
-    NumArgs = D.NumArgs;
   }
 
-  DiagnosticBuilder &operator=(const DiagnosticBuilder &) = delete;
-
-  /// Emits the diagnostic.
-  ~DiagnosticBuilder() {
-    Emit();
-  }
-
-  /// Forces the diagnostic to be emitted.
-  const DiagnosticBuilder &setForceEmit() const {
-    IsForceEmit = true;
+  template <typename T> const DiagnosticBuilder &operator<<(const T &V) const {
+    assert(isActive() && "Clients must not add to cleared diagnostic!");
+    const StreamingDiagnostic &DB = *this;
+    DB << V;
     return *this;
   }
 
-  /// Conversion of DiagnosticBuilder to bool always returns \c true.
-  ///
-  /// This allows is to be used in boolean error contexts (where \c true is
-  /// used to indicate that an error has occurred), like:
-  /// \code
-  /// return Diag(...);
-  /// \endcode
-  operator bool() const { return true; }
-
-  void AddString(StringRef S) const {
+  // It is necessary to limit this to rvalue reference to avoid calling this
+  // function with a bitfield lvalue argument since non-const reference to
+  // bitfield is not allowed.
+  template <typename T, typename = typename std::enable_if<
+                            !std::is_lvalue_reference<T>::value>::type>
+  const DiagnosticBuilder &operator<<(T &&V) const {
     assert(isActive() && "Clients must not add to cleared diagnostic!");
-    assert(NumArgs < DiagnosticsEngine::MaxArguments &&
-           "Too many arguments to diagnostic!");
-    DiagObj->DiagArgumentsKind[NumArgs] = DiagnosticsEngine::ak_std_string;
-    DiagObj->DiagArgumentsStr[NumArgs++] = std::string(S);
+    const StreamingDiagnostic &DB = *this;
+    DB << std::move(V);
+    return *this;
   }
 
-  void AddTaggedVal(intptr_t V, DiagnosticsEngine::ArgumentKind Kind) const {
-    assert(isActive() && "Clients must not add to cleared diagnostic!");
-    assert(NumArgs < DiagnosticsEngine::MaxArguments &&
-           "Too many arguments to diagnostic!");
-    DiagObj->DiagArgumentsKind[NumArgs] = Kind;
-    DiagObj->DiagArgumentsVal[NumArgs++] = V;
-  }
+  DiagnosticBuilder &operator=(const DiagnosticBuilder &) = delete;
 
-  void AddSourceRange(const CharSourceRange &R) const {
-    assert(isActive() && "Clients must not add to cleared diagnostic!");
-    DiagObj->DiagRanges.push_back(R);
-  }
+  /// Emits the diagnostic.
+  ~DiagnosticBuilder() { Emit(); }
 
-  void AddFixItHint(const FixItHint &Hint) const {
-    assert(isActive() && "Clients must not add to cleared diagnostic!");
-    if (!Hint.isNull())
-      DiagObj->DiagFixItHints.push_back(Hint);
+  /// Forces the diagnostic to be emitted.
+  const DiagnosticBuilder &setForceEmit() const {
+    IsForceEmit = true;
+    return *this;
   }
 
   void addFlagValue(StringRef V) const { DiagObj->FlagValue = std::string(V); }
 };
 
 struct AddFlagValue {
   StringRef Val;
 
   explicit AddFlagValue(StringRef V) : Val(V) {}
 };
 
 /// Register a value for the flag in the current diagnostic. This
 /// value will be shown as the suffix "=value" after the flag name. It is
 /// useful in cases where the diagnostic flag accepts values (e.g.,
 /// -Rpass or -Wframe-larger-than).
 inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
                                            const AddFlagValue V) {
   DB.addFlagValue(V.Val);
   return DB;
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                              StringRef S) {
   DB.AddString(S);
   return DB;
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                              const char *Str) {
   DB.AddTaggedVal(reinterpret_cast<intptr_t>(Str),
                   DiagnosticsEngine::ak_c_string);
   return DB;
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB, int I) {
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
+                                             int I) {
   DB.AddTaggedVal(I, DiagnosticsEngine::ak_sint);
   return DB;
 }
 
 // We use enable_if here to prevent that this overload is selected for
 // pointers or other arguments that are implicitly convertible to bool.
 template <typename T>
-inline std::enable_if_t<std::is_same<T, bool>::value, const DiagnosticBuilder &>
-operator<<(const DiagnosticBuilder &DB, T I) {
+inline std::enable_if_t<std::is_same<T, bool>::value,
+                        const StreamingDiagnostic &>
+operator<<(const StreamingDiagnostic &DB, T I) {
   DB.AddTaggedVal(I, DiagnosticsEngine::ak_sint);
   return DB;
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                              unsigned I) {
   DB.AddTaggedVal(I, DiagnosticsEngine::ak_uint);
   return DB;
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                              tok::TokenKind I) {
   DB.AddTaggedVal(static_cast<unsigned>(I), DiagnosticsEngine::ak_tokenkind);
   return DB;
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                              const IdentifierInfo *II) {
   DB.AddTaggedVal(reinterpret_cast<intptr_t>(II),
                   DiagnosticsEngine::ak_identifierinfo);
   return DB;
 }
 
 // Adds a DeclContext to the diagnostic. The enable_if template magic is here
 // so that we only match those arguments that are (statically) DeclContexts;
 // other arguments that derive from DeclContext (e.g., RecordDecls) will not
 // match.
 template <typename T>
 inline std::enable_if_t<
     std::is_same<std::remove_const_t<T>, DeclContext>::value,
-    const DiagnosticBuilder &>
-operator<<(const DiagnosticBuilder &DB, T *DC) {
+    const StreamingDiagnostic &>
+operator<<(const StreamingDiagnostic &DB, T *DC) {
   DB.AddTaggedVal(reinterpret_cast<intptr_t>(DC),
                   DiagnosticsEngine::ak_declcontext);
   return DB;
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                              SourceRange R) {
   DB.AddSourceRange(CharSourceRange::getTokenRange(R));
   return DB;
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                              ArrayRef<SourceRange> Ranges) {
   for (SourceRange R : Ranges)
     DB.AddSourceRange(CharSourceRange::getTokenRange(R));
   return DB;
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                              const CharSourceRange &R) {
   DB.AddSourceRange(R);
   return DB;
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                              const FixItHint &Hint) {
   DB.AddFixItHint(Hint);
   return DB;
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                              ArrayRef<FixItHint> Hints) {
   for (const FixItHint &Hint : Hints)
     DB.AddFixItHint(Hint);
   return DB;
 }
 
-inline const DiagnosticBuilder &
-operator<<(const DiagnosticBuilder &DB,
+inline const StreamingDiagnostic &
+operator<<(const StreamingDiagnostic &DB,
            const llvm::Optional<SourceRange> &Opt) {
   if (Opt)
     DB << *Opt;
   return DB;
 }
 
-inline const DiagnosticBuilder &
-operator<<(const DiagnosticBuilder &DB,
+inline const StreamingDiagnostic &
+operator<<(const StreamingDiagnostic &DB,
            const llvm::Optional<CharSourceRange> &Opt) {
   if (Opt)
     DB << *Opt;
   return DB;
 }
 
-inline const DiagnosticBuilder &
-operator<<(const DiagnosticBuilder &DB, const llvm::Optional<FixItHint> &Opt) {
+inline const StreamingDiagnostic &
+operator<<(const StreamingDiagnostic &DB,
+           const llvm::Optional<FixItHint> &Opt) {
   if (Opt)
     DB << *Opt;
   return DB;
 }
 
 /// A nullability kind paired with a bit indicating whether it used a
 /// context-sensitive keyword.
 using DiagNullabilityKind = std::pair<NullabilityKind, bool>;
 
-const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
+const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                       DiagNullabilityKind nullability);
 
 inline DiagnosticBuilder DiagnosticsEngine::Report(SourceLocation Loc,
                                                    unsigned DiagID) {
   assert(CurDiagID == std::numeric_limits<unsigned>::max() &&
          "Multiple diagnostics in flight at once!");
   CurDiagLoc = Loc;
   CurDiagID = DiagID;
   FlagValue.clear();
   return DiagnosticBuilder(this);
 }
 
-const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
+const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                       llvm::Error &&E);
 
 inline DiagnosticBuilder DiagnosticsEngine::Report(unsigned DiagID) {
   return Report(SourceLocation(), DiagID);
 }
 
 //===----------------------------------------------------------------------===//
 // Diagnostic
 //===----------------------------------------------------------------------===//
Diagnostic(const DiagnosticsEngine *DO, StringRef storedDiagMessage)
       : DiagObj(DO), StoredDiagMessage(storedDiagMessage) {}
 
   const DiagnosticsEngine *getDiags() const { return DiagObj; }
   unsigned getID() const { return DiagObj->CurDiagID; }
   const SourceLocation &getLocation() const { return DiagObj->CurDiagLoc; }
   bool hasSourceManager() const { return DiagObj->hasSourceManager(); }
   SourceManager &getSourceManager() const { return DiagObj->getSourceManager();}
 
-  unsigned getNumArgs() const { return DiagObj->NumDiagArgs; }
+  unsigned getNumArgs() const { return DiagObj->DiagStorage.NumDiagArgs; }
 
   /// Return the kind of the specified index.
   ///
   /// Based on the kind of argument, the accessors below can be used to get
   /// the value.
   ///
   /// \pre Idx < getNumArgs()
   DiagnosticsEngine::ArgumentKind getArgKind(unsigned Idx) const {
     assert(Idx < getNumArgs() && "Argument index out of range!");
-    return (DiagnosticsEngine::ArgumentKind)DiagObj->DiagArgumentsKind[Idx];
+    return (DiagnosticsEngine::ArgumentKind)
+        DiagObj->DiagStorage.DiagArgumentsKind[Idx];
   }
 
   /// Return the provided argument string specified by \p Idx.
   /// \pre getArgKind(Idx) == DiagnosticsEngine::ak_std_string
   const std::string &getArgStdStr(unsigned Idx) const {
     assert(getArgKind(Idx) == DiagnosticsEngine::ak_std_string &&
            "invalid argument accessor!");
-    return DiagObj->DiagArgumentsStr[Idx];
+    return DiagObj->DiagStorage.DiagArgumentsStr[Idx];
   }
 
   /// Return the specified C string argument.
   /// \pre getArgKind(Idx) == DiagnosticsEngine::ak_c_string
   const char *getArgCStr(unsigned Idx) const {
     assert(getArgKind(Idx) == DiagnosticsEngine::ak_c_string &&
            "invalid argument accessor!");
-    return reinterpret_cast<const char*>(DiagObj->DiagArgumentsVal[Idx]);
+    return reinterpret_cast<const char *>(
+        DiagObj->DiagStorage.DiagArgumentsVal[Idx]);
   }
 
   /// Return the specified signed integer argument.
   /// \pre getArgKind(Idx) == DiagnosticsEngine::ak_sint
   int getArgSInt(unsigned Idx) const {
     assert(getArgKind(Idx) == DiagnosticsEngine::ak_sint &&
            "invalid argument accessor!");
-    return (int)DiagObj->DiagArgumentsVal[Idx];
+    return (int)DiagObj->DiagStorage.DiagArgumentsVal[Idx];
   }
 
   /// Return the specified unsigned integer argument.
   /// \pre getArgKind(Idx) == DiagnosticsEngine::ak_uint
   unsigned getArgUInt(unsigned Idx) const {
     assert(getArgKind(Idx) == DiagnosticsEngine::ak_uint &&
            "invalid argument accessor!");
-    return (unsigned)DiagObj->DiagArgumentsVal[Idx];
+    return (unsigned)DiagObj->DiagStorage.DiagArgumentsVal[Idx];
   }
 
   /// Return the specified IdentifierInfo argument.
   /// \pre getArgKind(Idx) == DiagnosticsEngine::ak_identifierinfo
   const IdentifierInfo *getArgIdentifier(unsigned Idx) const {
     assert(getArgKind(Idx) == DiagnosticsEngine::ak_identifierinfo &&
            "invalid argument accessor!");
-    return reinterpret_cast<IdentifierInfo*>(DiagObj->DiagArgumentsVal[Idx]);
+    return reinterpret_cast<IdentifierInfo *>(
+        DiagObj->DiagStorage.DiagArgumentsVal[Idx]);
   }
 
   /// Return the specified non-string argument in an opaque form.
   /// \pre getArgKind(Idx) != DiagnosticsEngine::ak_std_string
   intptr_t getRawArg(unsigned Idx) const {
     assert(getArgKind(Idx) != DiagnosticsEngine::ak_std_string &&
            "invalid argument accessor!");
-    return DiagObj->DiagArgumentsVal[Idx];
+    return DiagObj->DiagStorage.DiagArgumentsVal[Idx];
   }
 
   /// Return the number of source ranges associated with this diagnostic.
   unsigned getNumRanges() const {
-    return DiagObj->DiagRanges.size();
+    return DiagObj->DiagStorage.DiagRanges.size();
   }
 
   /// \pre Idx < getNumRanges()
   const CharSourceRange &getRange(unsigned Idx) const {
     assert(Idx < getNumRanges() && "Invalid diagnostic range index!");
-    return DiagObj->DiagRanges[Idx];
+    return DiagObj->DiagStorage.DiagRanges[Idx];
   }
 
   /// Return an array reference for this diagnostic's ranges.
   ArrayRef<CharSourceRange> getRanges() const {
-    return DiagObj->DiagRanges;
+    return DiagObj->DiagStorage.DiagRanges;
   }
 
   unsigned getNumFixItHints() const {
-    return DiagObj->DiagFixItHints.size();
+    return DiagObj->DiagStorage.FixItHints.size();
   }
 
   const FixItHint &getFixItHint(unsigned Idx) const {
     assert(Idx < getNumFixItHints() && "Invalid index!");
-    return DiagObj->DiagFixItHints[Idx];
+    return DiagObj->DiagStorage.FixItHints[Idx];
   }
 
   ArrayRef<FixItHint> getFixItHints() const {
-    return DiagObj->DiagFixItHints;
+    return DiagObj->DiagStorage.FixItHints;
   }
 
   /// Format this diagnostic into a string, substituting the
   /// formal arguments into the %0 slots.
   ///
   /// The result is appended onto the \p OutStr array.
   void FormatDiagnostic(SmallVectorImpl<char> &OutStr) const;
 

clang/include/clang/Basic/PartialDiagnostic.h

 #include <type_traits>
 #include <utility>
 
 namespace clang {
 
 class DeclContext;
 class IdentifierInfo;
 
-class PartialDiagnostic {
-public:
-  enum {
-      // The MaxArguments and MaxFixItHints member enum values from
-      // DiagnosticsEngine are private but DiagnosticsEngine declares
-      // PartialDiagnostic a friend.  These enum values are redeclared
-      // here so that the nested Storage class below can access them.
-      MaxArguments = DiagnosticsEngine::MaxArguments
-  };
-
-  struct Storage {
-    enum {
-        /// The maximum number of arguments we can hold. We
-        /// currently only support up to 10 arguments (%0-%9).
-        ///
-        /// A single diagnostic with more than that almost certainly has to
-        /// be simplified anyway.
-        MaxArguments = PartialDiagnostic::MaxArguments
-    };
-
-    /// The number of entries in Arguments.
-    unsigned char NumDiagArgs = 0;
-
-    /// Specifies for each argument whether it is in DiagArgumentsStr
-    /// or in DiagArguments.
-    unsigned char DiagArgumentsKind[MaxArguments];
-
-    /// The values for the various substitution positions.
-    ///
-    /// This is used when the argument is not an std::string. The specific value
-    /// is mangled into an intptr_t and the interpretation depends on exactly
-    /// what sort of argument kind it is.
-    intptr_t DiagArgumentsVal[MaxArguments];
-
-    /// The values for the various substitution positions that have
-    /// string arguments.
-    std::string DiagArgumentsStr[MaxArguments];
-
-    /// The list of ranges added to this diagnostic.
-    SmallVector<CharSourceRange, 8> DiagRanges;
-
-    /// If valid, provides a hint with some code to insert, remove, or
-    /// modify at a particular position.
-    SmallVector<FixItHint, 6>  FixItHints;
-
-    Storage() = default;
-  };
-
-  /// An allocator for Storage objects, which uses a small cache to
-  /// objects, used to reduce malloc()/free() traffic for partial diagnostics.
-  class StorageAllocator {
-    static const unsigned NumCached = 16;
-    Storage Cached[NumCached];
-    Storage *FreeList[NumCached];
-    unsigned NumFreeListEntries;
-
-  public:
-    StorageAllocator();
-    ~StorageAllocator();
-
-    /// Allocate new storage.
-    Storage *Allocate() {
-      if (NumFreeListEntries == 0)
-        return new Storage;
-
-      Storage *Result = FreeList[--NumFreeListEntries];
-      Result->NumDiagArgs = 0;
-      Result->DiagRanges.clear();
-      Result->FixItHints.clear();
-      return Result;
-    }
-
-    /// Free the given storage object.
-    void Deallocate(Storage *S) {
-      if (S >= Cached && S <= Cached + NumCached) {
-        FreeList[NumFreeListEntries++] = S;
-        return;
-      }
-
-      delete S;
-    }
-  };
-
+class PartialDiagnostic : public StreamingDiagnostic {
 private:
   // NOTE: Sema assumes that PartialDiagnostic is location-invariant
   // in the sense that its bits can be safely memcpy'ed and destructed
   // in the new location.
 
   /// The diagnostic ID.
   mutable unsigned DiagID = 0;
-
-  /// Storage for args and ranges.
-  mutable Storage *DiagStorage = nullptr;
-
-  /// Allocator used to allocate storage for this diagnostic.
-  StorageAllocator *Allocator = nullptr;
-
-  /// Retrieve storage for this particular diagnostic.
-  Storage *getStorage() const {
-    if (DiagStorage)
-      return DiagStorage;
-
-    if (Allocator)
-      DiagStorage = Allocator->Allocate();
-    else {
-      assert(Allocator != reinterpret_cast<StorageAllocator *>(~uintptr_t(0)));
-      DiagStorage = new Storage;
-    }
-    return DiagStorage;
-  }
-
-  void freeStorage() {
-    if (!DiagStorage)
-      return;
-
-    // The hot path for PartialDiagnostic is when we just used it to wrap an ID
-    // (typically so we have the flexibility of passing a more complex
-    // diagnostic into the callee, but that does not commonly occur).
-    //
-    // Split this out into a slow function for silly compilers (*cough*) which
-    // can't do decent partial inlining.
-    freeStorageSlow();
-  }
-
-  void freeStorageSlow() {
-    if (Allocator)
-      Allocator->Deallocate(DiagStorage);
-    else if (Allocator != reinterpret_cast<StorageAllocator *>(~uintptr_t(0)))
-      delete DiagStorage;
-    DiagStorage = nullptr;
-  }
-
-  void AddSourceRange(const CharSourceRange &R) const {
-    if (!DiagStorage)
-      DiagStorage = getStorage();
-
-    DiagStorage->DiagRanges.push_back(R);
-  }
-
-  void AddFixItHint(const FixItHint &Hint) const {
-    if (Hint.isNull())
-      return;
-
-    if (!DiagStorage)
-      DiagStorage = getStorage();
-
-    DiagStorage->FixItHints.push_back(Hint);
-  }
-
 public:
   struct NullDiagnostic {};
 
   /// Create a null partial diagnostic, which cannot carry a payload,
   /// and only exists to be swapped with a real partial diagnostic.
   PartialDiagnostic(NullDiagnostic) {}
 
-  PartialDiagnostic(unsigned DiagID, StorageAllocator &Allocator)
-      : DiagID(DiagID), Allocator(&Allocator) {}
+  PartialDiagnostic(unsigned DiagID, DiagStorageAllocator &Allocator_)
+      : StreamingDiagnostic(Allocator_), DiagID(DiagID) {}
 
-  PartialDiagnostic(const PartialDiagnostic &Other)
-      : DiagID(Other.DiagID), Allocator(Other.Allocator) {
+  PartialDiagnostic(const PartialDiagnostic &Other) : DiagID(Other.DiagID) {
+    Allocator = Other.Allocator;
     if (Other.DiagStorage) {
       DiagStorage = getStorage();
       *DiagStorage = *Other.DiagStorage;
     }
   }
 
-  PartialDiagnostic(PartialDiagnostic &&Other)
-      : DiagID(Other.DiagID), DiagStorage(Other.DiagStorage),
-        Allocator(Other.Allocator) {
+  template <typename T> const PartialDiagnostic &operator<<(const T &V) const {
+    const StreamingDiagnostic &DB = *this;
+    DB << V;
+    return *this;
+  }
+
+  // It is necessary to limit this to rvalue reference to avoid calling this
+  // function with a bitfield lvalue argument since non-const reference to
+  // bitfield is not allowed.
+  template <typename T, typename = typename std::enable_if<
+                            !std::is_lvalue_reference<T>::value>::type>
+  const PartialDiagnostic &operator<<(T &&V) const {
+    const StreamingDiagnostic &DB = *this;
+    DB << std::move(V);
+    return *this;
+  }
+
+  PartialDiagnostic(PartialDiagnostic &&Other) : DiagID(Other.DiagID) {
+    Allocator = Other.Allocator;
+    DiagStorage = Other.DiagStorage;
     Other.DiagStorage = nullptr;
   }
 
-  PartialDiagnostic(const PartialDiagnostic &Other, Storage *DiagStorage)
-      : DiagID(Other.DiagID), DiagStorage(DiagStorage),
-        Allocator(reinterpret_cast<StorageAllocator *>(~uintptr_t(0))) {
+  PartialDiagnostic(const PartialDiagnostic &Other,
+                    DiagnosticStorage *DiagStorage_)
+      : DiagID(Other.DiagID) {
+    Allocator = reinterpret_cast<DiagStorageAllocator *>(~uintptr_t(0));
+    DiagStorage = DiagStorage_;
     if (Other.DiagStorage)
       *this->DiagStorage = *Other.DiagStorage;
   }
 
-  PartialDiagnostic(const Diagnostic &Other, StorageAllocator &Allocator)
-      : DiagID(Other.getID()), Allocator(&Allocator) {
+  PartialDiagnostic(const Diagnostic &Other, DiagStorageAllocator &Allocator_)
+      : DiagID(Other.getID()) {
+    Allocator = &Allocator_;
     // Copy arguments.
     for (unsigned I = 0, N = Other.getNumArgs(); I != N; ++I) {
       if (Other.getArgKind(I) == DiagnosticsEngine::ak_std_string)
         AddString(Other.getArgStdStr(I));
       else
         AddTaggedVal(Other.getRawArg(I), Other.getArgKind(I));
     }
 
PartialDiagnostic &operator=(PartialDiagnostic &&Other) {
     DiagID = Other.DiagID;
     DiagStorage = Other.DiagStorage;
     Allocator = Other.Allocator;
 
     Other.DiagStorage = nullptr;
     return *this;
   }
 
-  ~PartialDiagnostic() {
-    freeStorage();
-  }
-
   void swap(PartialDiagnostic &PD) {
     std::swap(DiagID, PD.DiagID);
     std::swap(DiagStorage, PD.DiagStorage);
     std::swap(Allocator, PD.Allocator);
   }
 
   unsigned getDiagID() const { return DiagID; }
 
-  void AddTaggedVal(intptr_t V, DiagnosticsEngine::ArgumentKind Kind) const {
-    if (!DiagStorage)
-      DiagStorage = getStorage();
-
-    assert(DiagStorage->NumDiagArgs < Storage::MaxArguments &&
-           "Too many arguments to diagnostic!");
-    DiagStorage->DiagArgumentsKind[DiagStorage->NumDiagArgs] = Kind;
-    DiagStorage->DiagArgumentsVal[DiagStorage->NumDiagArgs++] = V;
-  }
-
-  void AddString(StringRef V) const {
-    if (!DiagStorage)
-      DiagStorage = getStorage();
-
-    assert(DiagStorage->NumDiagArgs < Storage::MaxArguments &&
-           "Too many arguments to diagnostic!");
-    DiagStorage->DiagArgumentsKind[DiagStorage->NumDiagArgs]
-      = DiagnosticsEngine::ak_std_string;
-    DiagStorage->DiagArgumentsStr[DiagStorage->NumDiagArgs++] = std::string(V);
-  }
-
   void Emit(const DiagnosticBuilder &DB) const {
     if (!DiagStorage)
       return;
 
     // Add all arguments.
     for (unsigned i = 0, e = DiagStorage->NumDiagArgs; i != e; ++i) {
       if ((DiagnosticsEngine::ArgumentKind)DiagStorage->DiagArgumentsKind[i]
             == DiagnosticsEngine::ak_std_string)
public:
   }
 
   void EmitToString(DiagnosticsEngine &Diags,
                     SmallVectorImpl<char> &Buf) const {
     // FIXME: It should be possible to render a diagnostic to a string without
     //        messing with the state of the diagnostics engine.
     DiagnosticBuilder DB(Diags.Report(getDiagID()));
     Emit(DB);
-    DB.FlushCounts();
     Diagnostic(&Diags).FormatDiagnostic(Buf);
     DB.Clear();
     Diags.Clear();
   }
 
   /// Clear out this partial diagnostic, giving it a new diagnostic ID
   /// and removing all of its arguments, ranges, and fix-it hints.
   void Reset(unsigned DiagID = 0) {
     this->DiagID = DiagID;
     freeStorage();
   }
 
   bool hasStorage() const { return DiagStorage != nullptr; }
 
   /// Retrieve the string argument at the given index.
   StringRef getStringArg(unsigned I) {
     assert(DiagStorage && "No diagnostic storage?");
     assert(I < DiagStorage->NumDiagArgs && "Not enough diagnostic args");
     assert(DiagStorage->DiagArgumentsKind[I]
              == DiagnosticsEngine::ak_std_string && "Not a string arg");
     return DiagStorage->DiagArgumentsStr[I];
   }
-
-  friend const PartialDiagnostic &operator<<(const PartialDiagnostic &PD,
-                                             unsigned I) {
-    PD.AddTaggedVal(I, DiagnosticsEngine::ak_uint);
-    return PD;
-  }
-
-  friend const PartialDiagnostic &operator<<(const PartialDiagnostic &PD,
-                                             int I) {
-    PD.AddTaggedVal(I, DiagnosticsEngine::ak_sint);
-    return PD;
-  }
-
-  friend inline const PartialDiagnostic &operator<<(const PartialDiagnostic &PD,
-                                                    const char *S) {
-    PD.AddTaggedVal(reinterpret_cast<intptr_t>(S),
-                    DiagnosticsEngine::ak_c_string);
-    return PD;
-  }
-
-  friend inline const PartialDiagnostic &operator<<(const PartialDiagnostic &PD,
-                                                    StringRef S) {
-
-    PD.AddString(S);
-    return PD;
-  }
-
-  friend inline const PartialDiagnostic &operator<<(const PartialDiagnostic &PD,
-                                                    const IdentifierInfo *II) {
-    PD.AddTaggedVal(reinterpret_cast<intptr_t>(II),
-                    DiagnosticsEngine::ak_identifierinfo);
-    return PD;
-  }
-
-  // Adds a DeclContext to the diagnostic. The enable_if template magic is here
-  // so that we only match those arguments that are (statically) DeclContexts;
-  // other arguments that derive from DeclContext (e.g., RecordDecls) will not
-  // match.
-  template <typename T>
-  friend inline std::enable_if_t<std::is_same<T, DeclContext>::value,
-                                 const PartialDiagnostic &>
-  operator<<(const PartialDiagnostic &PD, T *DC) {
-    PD.AddTaggedVal(reinterpret_cast<intptr_t>(DC),
-                    DiagnosticsEngine::ak_declcontext);
-    return PD;
-  }
-
-  friend inline const PartialDiagnostic &operator<<(const PartialDiagnostic &PD,
-                                                    SourceRange R) {
-    PD.AddSourceRange(CharSourceRange::getTokenRange(R));
-    return PD;
-  }
-
-  friend inline const PartialDiagnostic &operator<<(const PartialDiagnostic &PD,
-                                                    const CharSourceRange &R) {
-    PD.AddSourceRange(R);
-    return PD;
-  }
-
-  friend const PartialDiagnostic &operator<<(const PartialDiagnostic &PD,
-                                             const FixItHint &Hint) {
-    PD.AddFixItHint(Hint);
-    return PD;
-  }
 };
 
 inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
                                            const PartialDiagnostic &PD) {
   PD.Emit(DB);
   return DB;
 }
 
 /// A partial diagnostic along with the source location where this
 /// diagnostic occurs.
 using PartialDiagnosticAt = std::pair<SourceLocation, PartialDiagnostic>;
 
 } // namespace clang
 
 #endif // LLVM_CLANG_BASIC_PARTIALDIAGNOSTIC_H

clang/include/clang/Sema/DelayedDiagnostic.h

public:
   /// A member declaration found through lookup.  The target is the
   /// member.
   enum MemberNonce { Member };
 
   /// A hierarchy (base-to-derived or derived-to-base) conversion.
   /// The target is the base class.
   enum BaseNonce { Base };
 
-  AccessedEntity(PartialDiagnostic::StorageAllocator &Allocator,
-                 MemberNonce _,
-                 CXXRecordDecl *NamingClass,
-                 DeclAccessPair FoundDecl,
-                 QualType BaseObjectType)
+  AccessedEntity(PartialDiagnostic::DiagStorageAllocator &Allocator,
+                 MemberNonce _, CXXRecordDecl *NamingClass,
+                 DeclAccessPair FoundDecl, QualType BaseObjectType)
       : Access(FoundDecl.getAccess()), IsMember(true),
         Target(FoundDecl.getDecl()), NamingClass(NamingClass),
-        BaseObjectType(BaseObjectType), Diag(0, Allocator) {
-  }
+        BaseObjectType(BaseObjectType), Diag(0, Allocator) {}
 
-  AccessedEntity(PartialDiagnostic::StorageAllocator &Allocator,
-                 BaseNonce _,
-                 CXXRecordDecl *BaseClass,
-                 CXXRecordDecl *DerivedClass,
-                 AccessSpecifier Access)
+  AccessedEntity(PartialDiagnostic::DiagStorageAllocator &Allocator,
+                 BaseNonce _, CXXRecordDecl *BaseClass,
+                 CXXRecordDecl *DerivedClass, AccessSpecifier Access)
       : Access(Access), IsMember(false), Target(BaseClass),
         NamingClass(DerivedClass), Diag(0, Allocator) {}
 
   bool isMemberAccess() const { return IsMember; }
 
   bool isQuiet() const { return Diag.getDiagID() == 0; }
 
   AccessSpecifier getAccess() const { return AccessSpecifier(Access); }

clang/include/clang/Sema/Ownership.h

struct PointerLikeTypeTraits<clang::OpaquePtr<T>> {
     }
   };
 
 } // namespace llvm
 
 namespace clang {
 
   // Basic
-  class DiagnosticBuilder;
+class StreamingDiagnostic;
 
-  // Determines whether the low bit of the result pointer for the
-  // given UID is always zero. If so, ActionResult will use that bit
-  // for it's "invalid" flag.
-  template<class Ptr>
-  struct IsResultPtrLowBitFree {
-    static const bool value = false;
+template <class Ptr> struct IsResultPtrLowBitFree {
   };
 
   /// ActionResult - This structure is used while parsing/acting on
   /// expressions, stmts, etc.  It encapsulates both the object returned by
   /// the action, plus a sense of whether or not it is valid.
   /// When CompressInvalid is true, the "invalid" flag will be
   /// stored in the low bit of the Val pointer.
   template<class PtrTy,
template <class Ptr> struct IsResultPtrLowBitFree {
   using ASTTemplateArgsPtr = MutableArrayRef<ParsedTemplateArgument>;
   using MultiTypeArg = MutableArrayRef<ParsedType>;
   using MultiTemplateParamsArg = MutableArrayRef<TemplateParameterList *>;
 
   inline ExprResult ExprError() { return ExprResult(true); }
   inline StmtResult StmtError() { return StmtResult(true); }
   inline TypeResult TypeError() { return TypeResult(true); }
 
-  inline ExprResult ExprError(const DiagnosticBuilder&) { return ExprError(); }
-  inline StmtResult StmtError(const DiagnosticBuilder&) { return StmtError(); }
+  inline ExprResult ExprError(const StreamingDiagnostic &) {
+    return ExprError();
+  }
+  inline StmtResult StmtError(const StreamingDiagnostic &) {
+    return StmtError();
+  }
 
   inline ExprResult ExprEmpty() { return ExprResult(false); }
   inline StmtResult StmtEmpty() { return StmtResult(false); }
 
   inline Expr *AssertSuccess(ExprResult R) {
     assert(!R.isInvalid() && "operation was asserted to never fail!");
     return R.get();
   }

clang/include/clang/Sema/ParsedAttr.h

enum AttributeDeclKind {
   ExpectedVariableOrField,
   ExpectedVariableFieldOrTag,
   ExpectedTypeOrNamespace,
   ExpectedFunctionVariableOrClass,
   ExpectedKernelFunction,
   ExpectedFunctionWithProtoType,
 };
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                              const ParsedAttr &At) {
   DB.AddTaggedVal(reinterpret_cast<intptr_t>(At.getAttrName()),
                   DiagnosticsEngine::ak_identifierinfo);
   return DB;
 }
 
-inline const PartialDiagnostic &operator<<(const PartialDiagnostic &PD,
-                                           const ParsedAttr &At) {
-  PD.AddTaggedVal(reinterpret_cast<intptr_t>(At.getAttrName()),
-                  DiagnosticsEngine::ak_identifierinfo);
-  return PD;
-}
-
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
                                              const ParsedAttr *At) {
   DB.AddTaggedVal(reinterpret_cast<intptr_t>(At->getAttrName()),
                   DiagnosticsEngine::ak_identifierinfo);
   return DB;
 }
 
-inline const PartialDiagnostic &operator<<(const PartialDiagnostic &PD,
-                                           const ParsedAttr *At) {
-  PD.AddTaggedVal(reinterpret_cast<intptr_t>(At->getAttrName()),
-                  DiagnosticsEngine::ak_identifierinfo);
-  return PD;
-}
-
 } // namespace clang
 
 #endif // LLVM_CLANG_SEMA_ATTRIBUTELIST_H

clang/include/clang/Sema/Sema.h

public:
     // source anyway, so the user-defined ~SemaDiagnosticBuilder is a safe no-op
     // in that case anwyay.
     SemaDiagnosticBuilder(const SemaDiagnosticBuilder&) = default;
 
     ~SemaDiagnosticBuilder() {
       // If we aren't active, there is nothing to do.
       if (!isActive()) return;
 
-      // Otherwise, we need to emit the diagnostic. First flush the underlying
-      // DiagnosticBuilder data, and clear the diagnostic builder itself so it
-      // won't emit the diagnostic in its own destructor.
+      // Otherwise, we need to emit the diagnostic. First clear the diagnostic
+      // builder itself so it won't emit the diagnostic in its own destructor.
       //
       // This seems wasteful, in that as written the DiagnosticBuilder dtor will
       // do its own needless checks to see if the diagnostic needs to be
       // emitted. However, because we take care to ensure that the builder
       // objects never escape, a sufficiently smart compiler will be able to
       // eliminate that code.
-      FlushCounts();
       Clear();
 
       // Dispatch to Sema to emit the diagnostic.
       SemaRef.EmitCurrentDiagnostic(DiagID);
     }
 
     /// Teach operator<< to produce an object of the correct type.
     template<typename T>
     friend const SemaDiagnosticBuilder &operator<<(
         const SemaDiagnosticBuilder &Diag, const T &Value) {
       const DiagnosticBuilder &BaseDiag = Diag;
       BaseDiag << Value;
       return Diag;
     }
+
+    // It is necessary to limit this to rvalue reference to avoid calling this
+    // function with a bitfield lvalue argument since non-const reference to
+    // bitfield is not allowed.
+    template <typename T, typename = typename std::enable_if<
+                              !std::is_lvalue_reference<T>::value>::type>
+    const SemaDiagnosticBuilder &operator<<(T &&V) const {
+      const StreamableDiagnosticBase &DB = *this;
+      DB << std::move(V);
+      return *this;
+    }
   };
 
   /// Emit a diagnostic.
   SemaDiagnosticBuilder Diag(SourceLocation Loc, unsigned DiagID) {
     DiagnosticBuilder DB = Diags.Report(Loc, DiagID);
     return SemaDiagnosticBuilder(DB, *this, DiagID);
   }
 

clang/include/clang/Tooling/Refactoring/RefactoringRuleContext.h

private:
   const SourceManager &SM;
   /// An optional source selection range that's commonly used to represent
   /// a selection in an editor.
   SourceRange SelectionRange;
   /// An optional AST for the translation unit on which a refactoring action
   /// might operate on.
   ASTContext *AST = nullptr;
   /// The allocator for diagnostics.
-  PartialDiagnostic::StorageAllocator DiagStorage;
+  PartialDiagnostic::DiagStorageAllocator DiagStorage;
 
   // FIXME: Remove when memoized.
   std::unique_ptr<SelectedASTNode> ASTNodeSelection;
 };
 
 } // end namespace tooling
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_RULE_CONTEXT_H

clang/lib/AST/ASTContext.cpp

void ASTContext::getFunctionFeatureMap(llvm::StringMap<bool> &FeatureMap,
   }
 }
 
 OMPTraitInfo &ASTContext::getNewOMPTraitInfo() {
   OMPTraitInfoVector.emplace_back(new OMPTraitInfo());
   return *OMPTraitInfoVector.back();
 }
 
-const DiagnosticBuilder &
-clang::operator<<(const DiagnosticBuilder &DB,
-                  const ASTContext::SectionInfo &Section) {
+const StreamingDiagnostic &clang::
+operator<<(const StreamingDiagnostic &DB,
   if (Section.Decl)
     return DB << Section.Decl;
   return DB << "a prior #pragma section";
 }
 
 bool ASTContext::mayExternalizeStaticVar(const Decl *D) const {
   return !getLangOpts().GPURelocatableDeviceCode &&
          ((D->hasAttr<CUDADeviceAttr>() &&

clang/lib/AST/DeclBase.cpp

DependentDiagnostic *DependentDiagnostic::Create(ASTContext &C,
   Parent = Parent->getPrimaryContext();
   if (!Parent->LookupPtr)
     Parent->CreateStoredDeclsMap(C);
 
   auto *Map = static_cast<DependentStoredDeclsMap *>(Parent->LookupPtr);
 
   // Allocate the copy of the PartialDiagnostic via the ASTContext's
   // BumpPtrAllocator, rather than the ASTContext itself.
-  PartialDiagnostic::Storage *DiagStorage = nullptr;
+  DiagnosticStorage *DiagStorage = nullptr;
   if (PDiag.hasStorage())
-    DiagStorage = new (C) PartialDiagnostic::Storage;
+    DiagStorage = new (C) DiagnosticStorage;
 
   auto *DD = new (C) DependentDiagnostic(PDiag, DiagStorage);
 
   // TODO: Maybe we shouldn't reverse the order during insertion.
   DD->NextDiagnostic = Map->FirstDiagnostic;
   Map->FirstDiagnostic = DD;
 
   return DD;
 }

clang/lib/AST/DeclCXX.cpp

switch (AS) {
     case AS_private:
       return "private";
     case AS_protected:
       return "protected";
   }
   llvm_unreachable("Invalid access specifier!");
 }
 
-const DiagnosticBuilder &clang::operator<<(const DiagnosticBuilder &DB,
-                                           AccessSpecifier AS) {
-  return DB << getAccessName(AS);
-}
-
-const PartialDiagnostic &clang::operator<<(const PartialDiagnostic &DB,
+const StreamingDiagnostic &clang::operator<<(const StreamingDiagnostic &DB,
                                              AccessSpecifier AS) {
   return DB << getAccessName(AS);
 }

clang/lib/AST/TemplateBase.cpp

SourceRange TemplateArgumentLoc::getSourceRange() const {
   case TemplateArgument::Pack:
   case TemplateArgument::Null:
     return SourceRange();
   }
 
   llvm_unreachable("Invalid TemplateArgument Kind!");
 }
 
-const DiagnosticBuilder &clang::operator<<(const DiagnosticBuilder &DB,
-                                           const TemplateArgument &Arg) {
+template <typename T>
+static const T &DiagTemplateArg(const T &DB, const TemplateArgument &Arg) {
   switch (Arg.getKind()) {
   case TemplateArgument::Null:
     // This is bad, but not as bad as crashing because of argument
     // count mismatches.
     return DB << "(null template argument)";
 
   case TemplateArgument::Type:
     return DB << Arg.getAsType();
case TemplateArgument::Pack: {
     Arg.print(Policy, OS);
     return DB << OS.str();
   }
   }
 
   llvm_unreachable("Invalid TemplateArgument Kind!");
 }
 
+const StreamingDiagnostic &clang::operator<<(const StreamingDiagnostic &DB,
+                                             const TemplateArgument &Arg) {
+  return DiagTemplateArg(DB, Arg);
+}
+
 clang::TemplateArgumentLocInfo::TemplateArgumentLocInfo(
     ASTContext &Ctx, NestedNameSpecifierLoc QualifierLoc,
     SourceLocation TemplateNameLoc, SourceLocation EllipsisLoc) {
   TemplateTemplateArgLocInfo *Template = new (Ctx) TemplateTemplateArgLocInfo;
   Template->Qualifier = QualifierLoc.getNestedNameSpecifier();
   Template->QualifierLocData = QualifierLoc.getOpaqueData();
   Template->TemplateNameLoc = TemplateNameLoc.getRawEncoding();
   Template->EllipsisLoc = EllipsisLoc.getRawEncoding();

clang/lib/AST/TemplateName.cpp

TemplateName::print(raw_ostream &OS, const PrintingPolicy &Policy,
   else if (AssumedTemplateStorage *Assumed = getAsAssumedTemplateName()) {
     Assumed->getDeclName().print(OS, Policy);
   } else {
     OverloadedTemplateStorage *OTS = getAsOverloadedTemplate();
     (*OTS->begin())->printName(OS);
   }
 }
 
-const DiagnosticBuilder &clang::operator<<(const DiagnosticBuilder &DB,
+const StreamingDiagnostic &clang::operator<<(const StreamingDiagnostic &DB,
                                              TemplateName N) {
   std::string NameStr;
   llvm::raw_string_ostream OS(NameStr);
   LangOptions LO;
   LO.CPlusPlus = true;
   LO.Bool = true;
   OS << '\'';
   N.print(OS, PrintingPolicy(LO));
   OS << '\'';
   OS.flush();
   return DB << NameStr;
 }
 
-const PartialDiagnostic&clang::operator<<(const PartialDiagnostic &PD,
-                                           TemplateName N) {
-  std::string NameStr;
-  llvm::raw_string_ostream OS(NameStr);
-  LangOptions LO;
-  LO.CPlusPlus = true;
-  LO.Bool = true;
-  OS << '\'';
-  N.print(OS, PrintingPolicy(LO));
-  OS << '\'';
-  OS.flush();
-  return PD << NameStr;
-}
-
 void TemplateName::dump(raw_ostream &OS) const {
   LangOptions LO;  // FIXME!
   LO.CPlusPlus = true;
   LO.Bool = true;
   print(OS, PrintingPolicy(LO));
 }
 
 LLVM_DUMP_METHOD void TemplateName::dump() const {
   dump(llvm::errs());
 }

clang/lib/Basic/Diagnostic.cpp

 #include <cstring>
 #include <limits>
 #include <string>
 #include <utility>
 #include <vector>
 
 using namespace clang;
 
-const DiagnosticBuilder &clang::operator<<(const DiagnosticBuilder &DB,
+const StreamingDiagnostic &clang::operator<<(const StreamingDiagnostic &DB,
                                              DiagNullabilityKind nullability) {
   StringRef string;
   switch (nullability.first) {
   case NullabilityKind::NonNull:
     string = nullability.second ? "'nonnull'" : "'_Nonnull'";
     break;
 
   case NullabilityKind::Nullable:
     string = nullability.second ? "'nullable'" : "'_Nullable'";
     break;
 
   case NullabilityKind::Unspecified:
     string = nullability.second ? "'null_unspecified'" : "'_Null_unspecified'";
     break;
   }
 
   DB.AddString(string);
   return DB;
 }
 
-const DiagnosticBuilder &clang::operator<<(const DiagnosticBuilder &DB,
+const StreamingDiagnostic &clang::operator<<(const StreamingDiagnostic &DB,
                                              llvm::Error &&E) {
   DB.AddString(toString(std::move(E)));
   return DB;
 }
 
 static void DummyArgToStringFn(DiagnosticsEngine::ArgumentKind AK, intptr_t QT,
                             StringRef Modifier, StringRef Argument,
                             ArrayRef<DiagnosticsEngine::ArgumentValue> PrevArgs,
                             SmallVectorImpl<char> &Output,

 }
 
 void DiagnosticsEngine::Report(const StoredDiagnostic &storedDiag) {
   assert(CurDiagID == std::numeric_limits<unsigned>::max() &&
          "Multiple diagnostics in flight at once!");
 
   CurDiagLoc = storedDiag.getLocation();
   CurDiagID = storedDiag.getID();
-  NumDiagArgs = 0;
+  DiagStorage.NumDiagArgs = 0;
 
-  DiagRanges.clear();
-  DiagRanges.append(storedDiag.range_begin(), storedDiag.range_end());
-
-  DiagFixItHints.clear();
-  DiagFixItHints.append(storedDiag.fixit_begin(), storedDiag.fixit_end());
+  DiagStorage.DiagRanges.clear();
+  DiagStorage.DiagRanges.append(storedDiag.range_begin(),
+                                storedDiag.range_end());
+
+  DiagStorage.FixItHints.clear();
+  DiagStorage.FixItHints.append(storedDiag.fixit_begin(),
+                                storedDiag.fixit_end());
 
   assert(Client && "DiagnosticConsumer not set!");
   Level DiagLevel = storedDiag.getLevel();
   Diagnostic Info(this, storedDiag.getMessage());
   Client->HandleDiagnostic(DiagLevel, Info);
   if (Client->IncludeInDiagnosticCounts()) {
     if (DiagLevel == DiagnosticsEngine::Warning)
       ++NumWarnings;
void ForwardingDiagnosticConsumer::clear() {
   DiagnosticConsumer::clear();
   Target.clear();
 }
 
 bool ForwardingDiagnosticConsumer::IncludeInDiagnosticCounts() const {
   return Target.IncludeInDiagnosticCounts();
 }
 
-PartialDiagnostic::StorageAllocator::StorageAllocator() {
+PartialDiagnostic::DiagStorageAllocator::DiagStorageAllocator() {
   for (unsigned I = 0; I != NumCached; ++I)
     FreeList[I] = Cached + I;
   NumFreeListEntries = NumCached;
 }
 
-PartialDiagnostic::StorageAllocator::~StorageAllocator() {
+PartialDiagnostic::DiagStorageAllocator::~DiagStorageAllocator() {
   // Don't assert if we are in a CrashRecovery context, as this invariant may
   // be invalidated during a crash.
   assert((NumFreeListEntries == NumCached ||
           llvm::CrashRecoveryContext::isRecoveringFromCrash()) &&
          "A partial is on the lam");
 }
 
 char DiagnosticError::ID;

clang/unittests/Basic/DiagnosticTest.cpp

for (unsigned FatalsAsError = 0; FatalsAsError != 2; ++FatalsAsError) {
     // The warning should be emitted and counted only if we're not suppressing
     // after fatal errors.
     EXPECT_EQ(Diags.getNumWarnings(), FatalsAsError);
   }
 }
 TEST(DiagnosticTest, diagnosticError) {
   DiagnosticsEngine Diags(new DiagnosticIDs(), new DiagnosticOptions,
                           new IgnoringDiagConsumer());
-  PartialDiagnostic::StorageAllocator Alloc;
+  PartialDiagnostic::DiagStorageAllocator Alloc;
   llvm::Expected<std::pair<int, int>> Value = DiagnosticError::create(
       SourceLocation(), PartialDiagnostic(diag::err_cannot_open_file, Alloc)
                             << "file"
                             << "error");
   ASSERT_TRUE(!Value);
   llvm::Error Err = Value.takeError();
   Optional<PartialDiagnosticAt> ErrDiag = DiagnosticError::take(Err);
   llvm::cantFail(std::move(Err));