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

[Pavel Labath via Phabricator via llvm-commits]

labath 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),
----------------
zturner wrote:
> 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.
I think Zachary answered the "copy" part very well.

As for unordered_map, my reason for doing that was two-fold:
- laziness to define DenseMapInfo for StreamType
- the fact that setting aside some values for the empty and tombstone keys this would render some minidumps invalid (although only a couple of dozen values of the entire 32-bit range is used, we must be prepared to handle any input)

However, neither of these two reasons is very strong, so I now define the DenseMapInfo traits, and also hard-reject any minidumps that contain one of the two stream types (to avoid asserting when trying to insert them into the map). We can always switch the special key values, if we find someone uses these for something else.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59291