[PATCH] D69471: [Coverage] Revise format to reduce binary size

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

Looks good as far as I can tell. I think it can be simplified by asserting that zlib::compress doesn't fail, though.



================
Comment at: llvm/docs/CoverageMappingFormat.rst:295
 
-It contains function name's MD5, the length of the encoded mapping data for that function, and function's 
-structural hash value.
-
-Encoded data:
--------------
-
-The encoded data is stored in a single string that contains
-the encoded filenames used by this translation unit and the encoded coverage
-mapping data for each function in this translation unit.
-
-The encoded data has the following structure:
-
-``[filenames, coverageMappingDataForFunctionRecord0, coverageMappingDataForFunctionRecord1, ..., padding]``
-
-If necessary, the encoded data is padded with zeroes so that the size
-of the data string is rounded up to the nearest multiple of 8 bytes.
+It contains function name's MD5, the length of the encoded mapping data for
+that function, the function's structural hash value, the hash of the filenames
----------------
missing 'the' before "function name's"?


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:727
+  using ThisTy = const FuncRecordTy *;
+  // Return function lookup key. The value is consider opaque.
+  template <support::endianness Endian> uint64_t getFuncNameRef() const {
----------------
s/consider/considered/


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:39
 
-void CoverageFilenamesSectionWriter::write(raw_ostream &OS) {
+Error CoverageFilenamesSectionWriter::write(raw_ostream &OS, bool Compress) {
+  std::string FilenamesStr;
----------------
It seems the only way this can fail is if zlib::compress fails, which should not be possible except if it runs out of memory, which we generally don't diagnose.

I think we could assert that zlib::compress succeeds, simplify the interface of this function, and remove the need for the new err_code_coverage_mapping_failed diag, as well as the compression_failed enumerator.


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

https://reviews.llvm.org/D69471