[PATCH] D84362: [NFC] Add missing functions to PartialDiagnostic

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D84362#2274315 <https://reviews.llvm.org/D84362#2274315>, @aaron.ballman wrote:

> In D84362#2271585 <https://reviews.llvm.org/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?


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

https://reviews.llvm.org/D84362