[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

1057–1061

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

1057–1061

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

177

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

[yaxunl]

clang/include/clang/Basic/PartialDiagnostic.h

177

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

1055

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

clang/include/clang/Basic/PartialDiagnostic.h

191

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

1055

renamed

clang/include/clang/Basic/PartialDiagnostic.h

191

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

191

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:
   /// 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 ASTContext::SectionInfo &Section);
+const PartialDiagnostic &operator<<(const PartialDiagnostic &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

struct ParsedTargetAttr {
   bool operator ==(const ParsedTargetAttr &Other) const {
     return DuplicateArchitecture == Other.DuplicateArchitecture &&
            Architecture == Other.Architecture && Features == Other.Features;
   }
 };
 
 #include "clang/AST/Attrs.inc"
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
-                                           const Attr *At) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &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,
-                                           CanQualType T) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &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) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &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) {
   PD.AddTaggedVal(reinterpret_cast<intptr_t>(ND),
                   DiagnosticsEngine::ak_nameddecl);

clang/include/clang/AST/DeclarationName.h

public:
   }
 
 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) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &DB,
   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,

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,
-                                           NestedNameSpecifier *NNS) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &DB,
   DB.AddTaggedVal(reinterpret_cast<intptr_t>(NNS),
                   DiagnosticsEngine::ak_nestednamespec);
   return DB;
 }
 
 } // namespace clang
 
 #endif // LLVM_CLANG_AST_NESTEDNAMESPECIFIER_H

clang/include/clang/AST/TemplateBase.h

struct alignas(void *) ASTTemplateKWAndArgsInfo {
   void initializeFrom(SourceLocation TemplateKWLoc);
 
   void copyInto(const TemplateArgumentLoc *ArgArray,
                 TemplateArgumentListInfo &List) const;
 };
 
 const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
                                     const TemplateArgument &Arg);
+const PartialDiagnostic &operator<<(const PartialDiagnostic &DB,
+                                    const TemplateArgument &Arg);
 
 inline TemplateSpecializationType::iterator
     TemplateSpecializationType::end() const {
   return getArgs() + getNumArgs();
 }
 
 inline DependentTemplateSpecializationType::iterator
     DependentTemplateSpecializationType::end() const {

clang/include/clang/AST/Type.h

inline const Type *Type::getPointeeOrArrayElementType() const {
   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) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &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,
                                            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) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &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,
                                            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) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &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,

clang/include/clang/Basic/Diagnostic.h

 // 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 {
   friend class DiagnosticsEngine;
   friend class PartialDiagnostic;
 
   mutable DiagnosticsEngine *DiagObj = nullptr;
   mutable unsigned NumArgs = 0;
 
   /// Status variable indicating if this diagnostic is still active.
public:
 
   void AddFixItHint(const FixItHint &Hint) const {
     assert(isActive() && "Clients must not add to cleared diagnostic!");
     if (!Hint.isNull())
       DiagObj->DiagFixItHints.push_back(Hint);
   }
 
   void addFlagValue(StringRef V) const { DiagObj->FlagValue = std::string(V); }
+  const static bool IsDiagBuilder = true;
 };
 
 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) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &DB,
   DB.addFlagValue(V.Val);
   return DB;
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
-                                           StringRef S) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &DB, StringRef S) {
   DB.AddString(S);
   return DB;
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
-                                           const char *Str) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &DB,
+                                      const std::string &S) {
+  DB.AddString(StringRef(S));
+  return DB;
+}
+
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &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) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &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) {
+template <typename DiagBuilderT, typename T>
+inline std::enable_if_t<std::is_same<T, bool>::value &&
+                            DiagBuilderT::IsDiagBuilder,
+                        const DiagBuilderT &>
+operator<<(const DiagBuilderT &DB, T I) {
   DB.AddTaggedVal(I, DiagnosticsEngine::ak_sint);
   return DB;
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
-                                           unsigned I) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &DB, unsigned I) {
   DB.AddTaggedVal(I, DiagnosticsEngine::ak_uint);
   return DB;
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
-                                           tok::TokenKind I) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &DB,
   DB.AddTaggedVal(static_cast<unsigned>(I), DiagnosticsEngine::ak_tokenkind);
   return DB;
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
-                                           const IdentifierInfo *II) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &DB,
   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>
+template <typename DiagBuilderT, 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) {
+    std::is_same<std::remove_const_t<T>, DeclContext>::value &&
+        DiagBuilderT::IsDiagBuilder,
+    const DiagBuilderT &>
+operator<<(const DiagBuilderT &DB, T *DC) {
   DB.AddTaggedVal(reinterpret_cast<intptr_t>(DC),
                   DiagnosticsEngine::ak_declcontext);
   return DB;
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
-                                           SourceRange R) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &DB, SourceRange R) {
   DB.AddSourceRange(CharSourceRange::getTokenRange(R));
   return DB;
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
-                                           ArrayRef<SourceRange> Ranges) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &DB,
   for (SourceRange R : Ranges)
     DB.AddSourceRange(CharSourceRange::getTokenRange(R));
   return DB;
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
-                                           const CharSourceRange &R) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &DB,
   DB.AddSourceRange(R);
   return DB;
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
-                                           const FixItHint &Hint) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &DB,
   DB.AddFixItHint(Hint);
   return DB;
 }
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
-                                           ArrayRef<FixItHint> Hints) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &DB,
   for (const FixItHint &Hint : Hints)
     DB.AddFixItHint(Hint);
   return DB;
 }
 
-inline const DiagnosticBuilder &
-operator<<(const DiagnosticBuilder &DB,
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &DB,
                                       const llvm::Optional<SourceRange> &Opt) {
   if (Opt)
     DB << *Opt;
   return DB;
 }
 
-inline const DiagnosticBuilder &
-operator<<(const DiagnosticBuilder &DB,
-           const llvm::Optional<CharSourceRange> &Opt) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &
+operator<<(const DiagBuilderT &DB, const llvm::Optional<CharSourceRange> &Opt) {
   if (Opt)
     DB << *Opt;
   return DB;
 }
 
-inline const DiagnosticBuilder &
-operator<<(const DiagnosticBuilder &DB, const llvm::Optional<FixItHint> &Opt) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &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,
-                                    DiagNullabilityKind nullability);
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+const DiagBuilderT &operator<<(const DiagBuilderT &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;
+}
 
 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();

clang/include/clang/Basic/PartialDiagnostic.h

private:
   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:
+  const static bool IsDiagBuilder = true;
   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(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) {
     Other.DiagStorage = nullptr;
   }
 
   PartialDiagnostic(const PartialDiagnostic &Other, Storage *DiagStorage)
       : DiagID(Other.DiagID), DiagStorage(DiagStorage),
         Allocator(reinterpret_cast<StorageAllocator *>(~uintptr_t(0))) {
     if (Other.DiagStorage)
       *this->DiagStorage = *Other.DiagStorage;
   }
 
   PartialDiagnostic(const Diagnostic &Other, StorageAllocator &Allocator)
       : DiagID(Other.getID()), Allocator(&Allocator) {
void AddString(StringRef V) const {
 
     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 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);
+  }
+
   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:
 
   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) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &DB,
   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/Ownership.h

namespace llvm {
   };
 
 } // namespace llvm
 
 namespace clang {
 
   // Basic
   class DiagnosticBuilder;
+  class PartialDiagnostic;
 
   // 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;
   };

clang/include/clang/Sema/ParsedAttr.h

enum AttributeDeclKind {
   ExpectedVariableOrField,
   ExpectedVariableFieldOrTag,
   ExpectedTypeOrNamespace,
   ExpectedFunctionVariableOrClass,
   ExpectedKernelFunction,
   ExpectedFunctionWithProtoType,
 };
 
-inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
-                                           const ParsedAttr &At) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &DB,
   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,
-                                           const ParsedAttr *At) {
+template <
+    typename DiagBuilderT,
+    typename std::enable_if<DiagBuilderT::IsDiagBuilder>::type * = nullptr>
+inline const DiagBuilderT &operator<<(const DiagBuilderT &DB,
   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/lib/AST/ASTContext.cpp

 
 const DiagnosticBuilder &
 clang::operator<<(const DiagnosticBuilder &DB,
                   const ASTContext::SectionInfo &Section) {
   if (Section.Decl)
     return DB << Section.Decl;
   return DB << "a prior #pragma section";
 }
+
+const PartialDiagnostic &clang::
+operator<<(const PartialDiagnostic &DB,
+           const ASTContext::SectionInfo &Section) {
+  if (Section.Decl)
+    return DB << Section.Decl;
+  return DB << "a prior #pragma section";
+}

clang/lib/AST/TemplateBase.cpp

 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/TemplateName.h"
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/LangOptions.h"
+#include "clang/Basic/PartialDiagnostic.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Compiler.h"
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 DiagnosticBuilder &clang::operator<<(const DiagnosticBuilder &DB,
+                                           const TemplateArgument &Arg) {
+  return DiagTemplateArg(DB, Arg);
+}
+
+const PartialDiagnostic &clang::operator<<(const PartialDiagnostic &DB,
+                                           const TemplateArgument &Arg) {
+  return DiagTemplateArg(DB, Arg);
+}
+
 const ASTTemplateArgumentListInfo *
 ASTTemplateArgumentListInfo::Create(const ASTContext &C,
                                     const TemplateArgumentListInfo &List) {
   std::size_t size = totalSizeToAlloc<TemplateArgumentLoc>(List.size());
   void *Mem = C.Allocate(size, alignof(ASTTemplateArgumentListInfo));
   return new (Mem) ASTTemplateArgumentListInfo(List);
 }
 

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,
-                                           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,
                                            llvm::Error &&E) {
   DB.AddString(toString(std::move(E)));
   return DB;
 }
 
 static void DummyArgToStringFn(DiagnosticsEngine::ArgumentKind AK, intptr_t QT,