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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
