[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/Basic/Diagnostic.h:1086-1090
 /// 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.
----------------
rnk wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84362