[PATCH] D20286: [Coverage] Fix an issue where improper coverage mapping data could be loaded for an inline function.

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/ProfileData/Coverage/CoverageMappingReader.cpp:445
@@ +444,3 @@
+        // not a dummy record and we don't need to replace it.
+        if (Records[RecordIndex].FunctionHash)
+          continue;
----------------
It is better to have another wrapper function to check dumminess which also takes FuncHash. The code can be simplified to

If (NewIsDummy || !OldIsDummy)
  continue

================
Comment at: lib/ProfileData/Coverage/CoverageMappingReader.cpp:474
@@ +473,3 @@
+          Filenames.size() - FilenamesBegin);
+      if (AlreadySeen) {
+        Records[RecordIndex] = FunctionRecord;
----------------
I think it is better to move duplicate resolution code above here (with a helper function) to improve readabiltity. The helper function can be something like:

InsertFunctionRecordIfNeeded (...)
 which does all the work of duplicate detection and existing entry update

{
   ..
   AlreadySeen = ..
   if (!AlreadySeen) {
       Records.push_back(..);
       return;
   }
   IsOldDummy = ..
   IsNewDummy = ..
   if (IsNewDummy || !IsOldDummy) 
      return;
  // Now replace old entry
  Records[OldIndex] = NewRecord;
}


http://reviews.llvm.org/D20286