[PATCH] D159141: [memprof] Add a MemProfReader base class.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Should there be a test to cover the new base class? In particular the constructor isn't called and the readNextRecord with a null Callback case.



================
Comment at: llvm/include/llvm/ProfileData/RawMemProfReader.h:69
+
+    if (Callback == nullptr) {
+      Callback =
----------------
nit: I think the braces can be removed in this case


================
Comment at: llvm/include/llvm/ProfileData/RawMemProfReader.h:86
+  // Initialize the MemProfReader with the frame mappings and profile contents.
+  MemProfReader(
+      llvm::DenseMap<FrameId, Frame> FrameIdMap,
----------------
I don't see this constructor being called anywhere in this patch?


================
Comment at: llvm/include/llvm/ProfileData/RawMemProfReader.h:147
   // Constructor for unittests only.
   RawMemProfReader(std::unique_ptr<llvm::symbolize::SymbolizableModule> Sym,
                    llvm::SmallVectorImpl<SegmentEntry> &Seg,
----------------
Should this call the base class constructor?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159141