[PATCH] D16133: [Coverage] : introduce class hierarchy (funcRecordReader) to support multiple versions of coverage data

[Vedant Kumar via llvm-commits]

vsk added a comment.

Thanks! Comments inline --


================
Comment at: lib/ProfileData/CoverageMappingReader.cpp:309
@@ -308,11 +308,3 @@
 
-template <typename T, support::endianness Endian>
-static std::error_code readCoverageMappingData(
-    InstrProfSymtab &ProfileNames, StringRef Data,
-    std::vector<BinaryCoverageReader::ProfileMappingRecord> &Records,
-    std::vector<StringRef> &Filenames) {
-  using namespace support;
-  llvm::DenseSet<T> UniqueFunctionMappingData;
-
-  // Read the records in the coverage data section.
-  for (const char *Buf = Data.data(), *End = Buf + Data.size(); Buf < End;) {
+struct CovMapFuncRecordReader {
+  virtual std::error_code readFunctionRecords(const char *&Buf,
----------------
This is a nice way to reduce the amount of template parameterization needed to pass around a reader object. However, since this class only used in this c++ file, maybe it would be better to get rid of this parent class and drop the 'Versioned' from 'VersionedCovMapFuncRecordReader'. Later, you can do `auto Reader = make_unique(...)`.

================
Comment at: lib/ProfileData/CoverageMappingReader.cpp:431
@@ +430,3 @@
+  // Read the records in the coverage data section.
+  std::unique_ptr<CovMapFuncRecordReader> Reader(nullptr);
+  for (const char *Buf = Data.data(), *End = Buf + Data.size(); Buf < End;) {
----------------
I'm not comfortable with `*&Buf` as an input-output parameter. I think `readFunctionRecords` should have direct access to the buffer, without needing it passed in. Could you pass `StringRef Data` into the Reader constructor?

================
Comment at: lib/ProfileData/CoverageMappingReader.cpp:433
@@ +432,3 @@
+  for (const char *Buf = Data.data(), *End = Buf + Data.size(); Buf < End;) {
+    auto CovHeader = reinterpret_cast<const coverage::CovMapHeader *>(Buf);
+    CoverageMappingVersion Version =
----------------
This should be lifted out of the loop. When the version bump happens, the line `if (Version > coverage::CoverageMappingCurrentVersion)` will start causing strange crashes :).


http://reviews.llvm.org/D16133