[PATCH] D29003: [OptDiag] Move everything but code region into a new base class for DiagnosticInfoOptimizationBase

[Matthias Braun via Phabricator via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.
This revision is now accepted and ready to land.

LGTM. Straightforward so not much to say except nitpicking about comments and names.



================
Comment at: include/llvm/Analysis/OptimizationDiagnosticInfo.h:71-73
+  ///
+  /// This takes a ...CommonBase rather than a ...Base because that is what
+  /// operator<< returns.
----------------
Probably no need to justify the choice of class here.

Instead I wish the documentation comment would tell me what the function actually does instead of just telling me it is the new way to emit things.


================
Comment at: include/llvm/IR/DiagnosticInfo.h:381
+/// that are used by both IR and MIR passes.
+class DiagnosticInfoOptimizationCommonBase
+    : public DiagnosticInfoWithDebugLocBase {
----------------
Why not call this `DiagnosticInfoOptimization` to make it a bit shorter and call the new thing `DiagnosticInfoIROptimization`/`DiagnosticInfoMIROptimization` (the fact that it is a `Base` class or `Common` is not that important that it needs to go into the name IMO).


================
Comment at: include/llvm/IR/DiagnosticInfo.h:478
+/// \brief Common features for diagnostics dealing with optimization remarks
+/// that are used IR passes.
+class DiagnosticInfoOptimizationBase
----------------
... used **by** (or in) IR passes


================
Comment at: lib/Analysis/OptimizationDiagnosticInfo.cpp:152
+    DiagnosticInfoOptimizationCommonBase &OptDiagCommon) {
+  auto *OptDiag = cast<DiagnosticInfoOptimizationBase>(&OptDiagCommon);
+  computeHotness(*OptDiag);
----------------
Could use a reference instead of a pointer.


================
Comment at: lib/Analysis/OptimizationDiagnosticInfo.cpp:157
   if (Out) {
-    auto *P = &const_cast<DiagnosticInfoOptimizationBase &>(OptDiag);
+    auto *P =
+        const_cast<DiagnosticInfoOptimizationCommonBase *>(&OptDiagCommon);
----------------
dito.


https://reviews.llvm.org/D29003