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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D84362#2274884 <https://reviews.llvm.org/D84362#2274884>, @tra wrote:

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


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

https://reviews.llvm.org/D84362