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

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish marked an inline comment as done.
snehasish added a comment.

In D159141#4626637 <https://reviews.llvm.org/D159141#4626637>, @tejohnson wrote:

> 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.

Added a test which uses the constructor and relies on the default null Callback behaviour.



================
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,
----------------
tejohnson wrote:
> I don't see this constructor being called anywhere in this patch?
Added a test which uses this constructor.


================
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,
----------------
tejohnson wrote:
> Should this call the base class constructor?
No we can't call the base class constructor from here since we need to do additional processing (`symbolizeAndFilterStackFrames` and `mapRawProfileToRecords`) before populating the function profile data structure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159141