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

[Igor Kudrin via llvm-commits]

ikudrin added inline comments.

================
Comment at: lib/ProfileData/Coverage/CoverageMappingReader.cpp:297
@@ -296,1 +296,3 @@
 
+Error RawCoverageMappingDummyChecker::isDummy(bool &Result) {
+  // A dummy coverage mapping data consists of just one region with zero count.
----------------
vsk wrote:
> nit, could you use `Expected<bool>` here? That should let you simplify this a bit.
Thanks! I'll update the patch tomorrow.

================
Comment at: lib/ProfileData/Coverage/CoverageMappingReader.cpp:351
@@ +350,3 @@
+  if (Hash) {
+    Result = false;
+    return Error::success();
----------------
davidxl wrote:
> Should this be moved above the if(Hash) so that caller does not need to initialize it.
The caller doesn't need to initialize it. If Hash is 0, RawCoverageMappingDummyChecker::isDummy() does its part of work.

================
Comment at: lib/ProfileData/Coverage/CoverageMappingReader.cpp:408
@@ +407,3 @@
+    }
+    // Update the existent record if it's a dummy and the new record is real.
+    size_t OldRecordIndex = InsertResult.first->second;
----------------
vsk wrote:
> nit, existing
Thanks!

================
Comment at: test/tools/llvm-cov/Inputs/prefer_used_to_unused.cpp:5
@@ +4,2 @@
+  return sampleFunc(5) + simpleFunc(5);
+}
----------------
davidxl wrote:
> do you need another source file that includes prefer_used_to_unused.h but does not call sampleFunc nor simpleFunc in order to force creation of dummies?
I use `prefer_used_to_unused.h` itself for this, see the comment at line 6 there.


http://reviews.llvm.org/D20286