This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Dor1s on Feb 28 2018, 1:50 PM.

Details

Summary

The option is helpful for large projects where it's not feasible to specify sources which
user would like to see in the report. Instead, it allows to black-list specific sources via
regular expressions (e.g. now it's possible to skip all files that have "test" in its name).

This also partially fixes https://bugs.llvm.org/show_bug.cgi?id=34277

Diff Detail

Repository
rL LLVM

Event Timeline

Dor1s created this revision.Feb 28 2018, 1:50 PM
Dor1s updated this revision to Diff 141017.Apr 4 2018, 12:16 PM

Still draft, in the middle of refactoring

Dor1s updated this revision to Diff 141198.Apr 5 2018, 12:39 PM

This version works, but I need to self-review it first.

Dor1s retitled this revision from Draft to [llvm-cov] Implement -ignore-source-regex= option for excluding source files..Apr 5 2018, 1:08 PM
Dor1s edited the summary of this revision. (Show Details)
Dor1s updated this revision to Diff 141210.Apr 5 2018, 1:26 PM
Dor1s edited the summary of this revision. (Show Details)

Minor fixes after self-review + update the documentation page.

Dor1s updated this revision to Diff 141212.Apr 5 2018, 1:34 PM

Use clang-format on the modified and added files.

Dor1s added a subscriber: kcc.

Please take a look when you get a chance :)

vsk added a comment.Apr 5 2018, 2:20 PM

Thanks for the patch.

Could you please leave out any whitespace, formatting, or grammar changes not related to -ignore-source-regex? It can help to use clang-format-diff.py as a git hook to avoid formatting churn. Also, why does CoverageExporter need to be split out of CoverageExporterJson for this patch? Could it be done as a follow-up instead?

Dor1s updated this revision to Diff 141226.Apr 5 2018, 3:16 PM

Revert clang-format and remove CoverageExporter.cpp which is unnecessary at this point.

Dor1s updated this revision to Diff 141227.Apr 5 2018, 3:19 PM

Finish revert of clang-format.

Dor1s added a comment.Apr 5 2018, 3:20 PM

Sure! I've reverted all changes that are not necessary for adding the new option.

vsk added inline comments.Apr 5 2018, 3:29 PM
tools/llvm-cov/CodeCoverage.cpp
604 ↗(On Diff #141227)

For consistency with -name-regex, could we name this 'ignore-filename-regex' and drop the period at the end of the description?

tools/llvm-cov/SourceFilters.h
23 ↗(On Diff #141227)

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.

Dor1s updated this revision to Diff 141439.Apr 6 2018, 3:25 PM
Dor1s marked 2 inline comments as done.

Address review comments: rename "source" to "filename" and move new logic into CoverageFilters.

Dor1s retitled this revision from [llvm-cov] Implement -ignore-source-regex= option for excluding source files. to [llvm-cov] Implement -ignore-filename-regex= option for excluding source files..Apr 6 2018, 3:27 PM
vsk accepted this revision.Apr 6 2018, 3:28 PM

Lgtm, thanks!

This revision is now accepted and ready to land.Apr 6 2018, 3:28 PM
Dor1s added a comment.Apr 6 2018, 3:37 PM

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 :)

tools/llvm-cov/CodeCoverage.cpp
604 ↗(On Diff #141227)

That sounds much better, thanks for the suggestion!

tools/llvm-cov/SourceFilters.h
23 ↗(On Diff #141227)

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;.

Dor1s updated this revision to Diff 141446.Apr 6 2018, 3:43 PM

Add one more test to ensure that ignore-filename-regex is applied even when SOURCES are specified.

vsk added a comment.Apr 6 2018, 4:06 PM

It does seem like the logic to get a filtered list of files could be factored out. Forwarding CodeCoverageTool into CoverageReport, etc. might be a good way to achieve that. Another option is to add a utility to CoverageViewOptions, which should already be threaded through.

Dor1s added a comment.Apr 9 2018, 8:23 AM
In D43907#1060440, @vsk wrote:

It does seem like the logic to get a filtered list of files could be factored out. Forwarding CodeCoverageTool into CoverageReport, etc. might be a good way to achieve that. Another option is to add a utility to CoverageViewOptions, which should already be threaded through.

Thanks Vedant for taking another look. To avoid further complications, I'm landing this, and will try some refactoring in a follow-up CL(s).

This revision was automatically updated to reflect the committed changes.