[PATCH] D63232: [Coverage] Load code coverage data from archives

[Max Moroz via Phabricator via llvm-commits]

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

Left some questions / suggestions, but don't see any problems, LGTM!



================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp:595
+  Reader->ProfileNames = std::move(ProfileNames);
+  if (BytesInAddress == 4 && Endian == support::endianness::little) {
+    if (Error E =
----------------
nit: I guess it's stupid, but maybe `sizeof(uint32_t)` isntead of `4`? And the same in the following conditions.


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp:597
+    if (Error E =
+            readCoverageMappingData<uint32_t, support::endianness::little>(
+                Reader->ProfileNames, Coverage, Reader->MappingRecords,
----------------
I think we can make it a bit more concise (2 branches instead of 4) by passing `Endian` as a template parameter. May need a separate check though to make sure it's either `support::endianness::little` or `support::endianness::big`.


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp:742
+      return ReaderOrErr.takeError();
+    Readers.emplace_back(std::move(ReaderOrErr.get()));
+    return Readers;
----------------
Why do we use `emplace_back` here and on line 794, but `push_back` in the other places?


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp:757
+      if (Arch != ObjArch)
+        continue;
+
----------------
Should we log anything here?


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

https://reviews.llvm.org/D63232