[PATCH] D101780: [CoverageMapping] Handle gaps in counter IDs for source-based coverage

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp:196
+  case Counter::Expression: {
+    if (C.getExpressionID() >= Expressions.size())
+      return 0;
----------------
I'm not sure how this case can arise. Would it be preferable to assert C.getExpressionID() < Expressions.size() instead?


================
Comment at: llvm/unittests/ProfileData/CoverageMappingTest.cpp:805
+// Test that non-contiguous counter ids are handled.
+TEST_P(CoverageMappingTest, non_contiguous_counter_ids) {
+  // No records in profdata
----------------
A bit of a nit, but: based on our discussion it seems like we want to allow counters to not be associated with code regions. That's not necessarily the same as allowing non-contiguous counter ID's (although it's fine that this patch does that).

I think it'd be helpful for the comments / test case name to reflect the former (maybe something like 'non_code_region_counter'): that'd leave the door open to disallowing non-contiguous counter ID's down the line.


================
Comment at: llvm/unittests/ProfileData/CoverageMappingTest.cpp:819
+  for (const auto &Func : LoadedCoverage->getCoveredFunctions())
+    Names.push_back(Func.Name);
+  ASSERT_EQ(1U, Names.size());
----------------
It would tighten the test up to check that the first FunctionRecord has a .CountedRegions field with the expected size.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101780