[PATCH] D156611: [llvm-cov] Support multi-source object files for `convert-for-testing`

[Gulfem Savrun Yeniceri via Phabricator via llvm-commits]

gulfem added a comment.

I added a few nit-picks. Other than that, looks good to me.



================
Comment at: llvm/docs/CoverageMappingFormat.rst:620-621
+
+``llvm-cov`` uses a special file format (called ``.covmapping`` below) for its
+regression tests to avoid including complete binary files to the LLVM
+repository. This format is private and should have no use for general users.
----------------
just say `for testing purposes` and remove the part about `including binary in the repository`.


================
Comment at: llvm/docs/CoverageMappingFormat.rst:657
+The only difference between Version1 and Version2 is in the encoding of the
+``coverageMapping`` fields, which we will talk about later.
+
----------------
`which is explained later.`


================
Comment at: llvm/docs/CoverageMappingFormat.rst:686
+superset of Version1. ``llvm-cov convert-for-testing`` will only generate
+Version2 files currently. The Version1 is kept for backward compatibility.
+
----------------
Just say `Version 2 is the current version`, and please remove the rest (no need to explain why Version 1 is kept, etc.).


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp:946
+    // Skip the padding bytes because coverage map data has an alignment of 8.
+    size_t Pad = offsetToAlignedAddr(Data.data(), Align(8));
+    if (Data.size() < Pad)
----------------
Some code is duplicated between the case statements, and each case statement is long to read. I think for checking versions `if` might be a better fit than `switch` because there might be several versions later. So, I would suggest this: when the `TestingFormatVersion` is bigger than `Version1`, use `decodeULEB128` to read the size. The rest of the code can mostly be shared up to this point:

```
uint32_t FilenamesSize =
CovHeader->getFilenamesSize<support::endianness::little>();
CoverageMappingSize = sizeof(CovMapHeader) + FilenamesSize;
```

You can put the code above in a `if` statement that checks `TestingFormatVersion`, and I think the rest can be shared.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156611