[PATCH] D19725: [Coverage] Fix an issue where a coverage region might not be created for a macro containing for or while statements.

[Justin Bogner via cfe-commits]

Igor Kudrin <ikudrin.dev at gmail.com> writes:
> ikudrin created this revision.
> ikudrin added reviewers: bogner, davidxl, vsk.
> ikudrin added a subscriber: cfe-commits.
>
> The situation happened when a macro contained a full loop statement,
> which body ended at the end of the macro.

A comment and a nitpick, then LGTM.

> http://reviews.llvm.org/D19725
>
> Files:
>   lib/CodeGen/CoverageMappingGen.cpp
>   test/CoverageMapping/macroscopes.cpp
>
> Index: test/CoverageMapping/macroscopes.cpp
> ===================================================================
> --- test/CoverageMapping/macroscopes.cpp
> +++ test/CoverageMapping/macroscopes.cpp
> @@ -22,6 +22,17 @@
>  #define starts_a_while while (x < 5)
>  #define simple_stmt ++x
>  
> +#define macro_with_for          \
> +  x = 3;                        \
> +  for (int i = 0; i < x; ++i) { \
> +  }
> +
> +#define macro_with_while \
> +  x = 4;                 \
> +  while (x < 5) {        \
> +    ++x;                 \
> +  }
> +
>  // CHECK: main
>  // CHECK-NEXT: File 0, [[@LINE+1]]:12 -> {{[0-9]+}}:2 = #0
>  int main() {
> @@ -64,6 +75,11 @@
>      simple_stmt;
>    ends_a_scope
>  
> +  // CHECK-NEXT: Expansion,File 0, [[@LINE+1]]:3 -> [[@LINE+1]]:17 = #0
> +  macro_with_for
> +  // CHECK-NEXT: Expansion,File 0, [[@LINE+1]]:3 -> [[@LINE+1]]:19 = #0
> +  macro_with_while
> +
>    return 0;
>  }
>  
> @@ -103,3 +119,10 @@
>  // CHECK-NEXT: File 11, 22:31 -> 22:36 = (#0 + #9)
>  // CHECK-NEXT: File 12, 23:21 -> 23:24 = #9
>  // CHECK-NEXT: File 13, 6:3 -> 7:4 = #9
> +// CHECK-NEXT: File 14, 26:3 -> 28:4 = #0
> +// CHECK-NEXT: File 14, 27:19 -> 27:24 = (#0 + #10)
> +// CHECK-NEXT: File 14, 27:26 -> 27:29 = #10
> +// CHECK-NEXT: File 14, 27:31 -> 28:4 = #10
> +// CHECK-NEXT: File 15, 31:3 -> 34:4 = #0
> +// CHECK-NEXT: File 15, 32:10 -> 32:15 = (#0 + #11)
> +// CHECK-NEXT: File 15, 32:17 -> 34:4 = #11
> Index: lib/CodeGen/CoverageMappingGen.cpp
> ===================================================================
> --- lib/CodeGen/CoverageMappingGen.cpp
> +++ lib/CodeGen/CoverageMappingGen.cpp
> @@ -443,15 +443,30 @@
>      return ExitCount;
>    }
>  
> +  /// \brief Check whether a region with bounds \c StartLoc and \c EndLoc
> +  /// is already added to \c SourceRegions.
> +  bool isRegionAlreadyAdded(SourceLocation StartLoc, SourceLocation EndLoc) {
> +    return SourceRegions.rend() !=
> +           std::find_if(SourceRegions.rbegin(), SourceRegions.rend(),

Any particular reason for rbegin/rend? I guess you expect the match to
be near the end of the list when it exists?

> +                        [=](const SourceMappingRegion &Region) {

There's no good reason to capture by value here. Best to use [&] for
efficiency.

> +                          assert(Region.hasStartLoc() && "incomplete region");
> +                          assert(Region.hasEndLoc() && "incomplete region");
> +                          return Region.getStartLoc() == StartLoc &&
> +                                 Region.getEndLoc() == EndLoc;
> +                        });
> +  }
> +
>    /// \brief Adjust the most recently visited location to \c EndLoc.
>    ///
>    /// This should be used after visiting any statements in non-source order.
>    void adjustForOutOfOrderTraversal(SourceLocation EndLoc) {
>      MostRecentLocation = EndLoc;
>      // Avoid adding duplicate regions if we have a completed region on the top
> -    // of the stack and are adjusting to the end of a virtual file.
> +    // of the stack.
>      if (getRegion().hasEndLoc() &&
> -        MostRecentLocation == getEndOfFileOrMacro(MostRecentLocation))
> +        MostRecentLocation == getEndOfFileOrMacro(MostRecentLocation) &&
> +        isRegionAlreadyAdded(getStartOfFileOrMacro(MostRecentLocation),
> +                             MostRecentLocation))
>        MostRecentLocation = getIncludeOrExpansionLoc(MostRecentLocation);
>    }
>