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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp:595
+  Reader->ProfileNames = std::move(ProfileNames);
+  if (BytesInAddress == 4 && Endian == support::endianness::little) {
+    if (Error E =
----------------
Dor1s wrote:
> nit: I guess it's stupid, but maybe `sizeof(uint32_t)` isntead of `4`? And the same in the following conditions.
If it were possible to write 'sizeof(intptr32_t)', I think that would be clearer. I'm not sure 'sizeof(uint32_t)' is clearer than '4', though.


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp:597
+    if (Error E =
+            readCoverageMappingData<uint32_t, support::endianness::little>(
+                Reader->ProfileNames, Coverage, Reader->MappingRecords,
----------------
Dor1s wrote:
> 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`.
As 'Endian' is not a constant-expression, I don't think it's possible to pass it in as a template parameter. I think it could be passed as a regular parameter, but that would eliminate two branches here while adding two branches to 'readCoverageMappingData'.


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


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp:757
+      if (Arch != ObjArch)
+        continue;
+
----------------
Dor1s wrote:
> Should we log anything here?
I don't think so. It's expected that at least one of the slices in a universal binary will not match the desired architecture. I'll add a comment to that effect.


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

https://reviews.llvm.org/D63232