[PATCH] D59291: [Object] Add basic minidump support

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: include/llvm/Object/Minidump.h:77
+               ArrayRef<minidump::Directory> Streams,
+               std::unordered_map<minidump::StreamType, std::size_t> StreamMap)
+      : Binary(ID_Minidump, Source), Header(Header), Streams(Streams),
----------------
jhenderson wrote:
> Are you deliberately making a copy of StreamMap? I would normally expect this to be passed by some form of reference.
It's actually not making a copy.  This gives the caller the ability to write `std::move(StreamMap)`, which will actually *prevent* a copy.  If you change this to a reference, then in order to store it in the class by value a copy must be made.  By passing it by value in the constructor, you can prevent *any* copies from being made, because the caller can move it in at construction time (and indeed, this is what happens later on).


However, I would also ask why we're using `std::unordered_map<>` instead of `llvm::DenseMap<>`, which is generally more efficient.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59291