[PATCH] D43907: [llvm-cov] Implement -ignore-filename-regex= option for excluding source files.

[Max Moroz via Phabricator via llvm-commits]

Dor1s added a comment.

Vedant, thanks for the awesome suggestions. I've addressed those.

Frankly, I don't like that I apply `IgnoreFilenameFilters` in 3 different places if `InputSourceFiles` are not specified. We might consider moving `auto Coverage = load();` out of `doShow / doReport / doExport`, but that would be inefficient when command specific options are wrong, i.e. command doesn't execute but we spend time loading the coverage.

Another idea I can think of is to create a method similar to `CodeCoverageTool::load`, something like:

  void CodeCoverageTool::loadSourceFilesIfNeeded(const coverage::CoverageMapping &Coverage) {
    if (SourceFiles.empty())
      // Get the source files from the function coverage mapping.
      for (StringRef Filename : Coverage->getUniqueSourceFiles()) {
        if (!IgnoreFilenameFilters.matchesFilename(Filename))
          SourceFiles.push_back(Filename);
      }
  }

And call it in every command implementation right after `auto Coverage = load();`

WDYT? `loadSourceFilesIfNeeded` is probably not a good name :)

And another idea is to just add that code to the `load()` method :)



================
Comment at: tools/llvm-cov/CodeCoverage.cpp:604
+  cl::list<std::string> IgnoreSourceRegexFilters(
+      "ignore-source-regex", cl::Optional,
+      cl::desc("Skip source code files with file paths that match the given "
----------------
vsk wrote:
> For consistency with -name-regex, could we name this 'ignore-filename-regex' and drop the period at the end of the description?
That sounds much better, thanks for the suggestion!


================
Comment at: tools/llvm-cov/SourceFilters.h:23
+/// \brief Matches specific filename that pass the requirement of this filter.
+class SourceFilter {
+public:
----------------
vsk wrote:
> Wdyt of implementing this by adding 'virtual bool matchesFilename(StringRef Filename)' to CoverageFilter? That should allow you to dedup some similar functionality as CoverageFilters already exists, etc. 
That's a great suggestion! Done! Looks like I may also want to rename existing `CoverageFiltersMatchAll Filters;` into `CoverageFiltersMatchAll FunctionNameFilters;` to be more explicit given that now there is `CoverageFilters IgnoreFilenameFilters;`.


Repository:
  rL LLVM

https://reviews.llvm.org/D43907