Page MenuHomePhabricator

[llvm-cov] Add the project summary to each source file coverage report.
ClosedPublic

Authored by MaggieYi on Aug 10 2016, 1:27 AM.

Details

Summary

This patch includes the following changes:

  • Included header "Code coverage report" and include the date that the report was created.
  • Included title (as specified in a command line option, (i.e llvm-cov -project-title="Simple Test")
  • In the summary, list the elf files that the source code file has contributed to.
  • Used column heading for "Line No.", "Count No.", Source".

Diff Detail

Repository
rL LLVM

Event Timeline

MaggieYi updated this revision to Diff 67482.Aug 10 2016, 1:27 AM
MaggieYi retitled this revision from to [llvm-cov] Add the project summary to each source file coverage report..
MaggieYi updated this object.
MaggieYi added reviewers: vsk, davidxl, bogner.
MaggieYi added a subscriber: llvm-commits.
vsk added inline comments.Aug 10 2016, 11:03 AM
tools/llvm-cov/CodeCoverage.cpp
467 ↗(On Diff #67482)

We should handle the error_code make_absolute returns (CodeCoverageTool::warning).

470 ↗(On Diff #67482)

We aren't calling sys::path::native in CoveragePrinter::getOutputPath() either. We should make the change to ObjectFilename and getOutputPath in the same patch, and make sure to test it with a covmapping file that includes Windows path separators. Could you tackle this in a separate patch?

627 ↗(On Diff #67482)

Use CodeCoverageTool::error.

632 ↗(On Diff #67482)

Please sink the global into CoverageViewOptions, so there's just one place we need to look for information like this.

633 ↗(On Diff #67482)

It's not clear that the results of the find+substr would be uniform across platforms. I suggest just using the result of MTime.str().

tools/llvm-cov/SourceCoverageView.h
175 ↗(On Diff #67482)

Nit about the comment: since this is a field, the word "return" seems out of place. Wdyt of "Specifies whether or not ..."?

201 ↗(On Diff #67482)

This method does not need to be pure-virtual. Please delete it and move its logic into the renderSourceName implementations.

252 ↗(On Diff #67482)

I think we should delete all three of these methods and replace them with something simpler: virtual void renderCellInTitle(raw_ostream &, StringRef CellText). The text viewer would provide an implementation that prints "CellText" in cyan followed by a newline. The html viewer would do something analogous.

This would force some common logic to move into SourceCoverageView::print, which is a good thing. E.g, we'd no longer have inconsistent capitalization between "renderReportTitle" in the text vs. html viewers.

255 ↗(On Diff #67482)

This isn't pure-virtual, so it doesn't belong in this section of SourceCoverageView. Please delete it and move its logic to SourceCoverageView::print.

tools/llvm-cov/SourceCoverageViewHTML.cpp
339 ↗(On Diff #67482)

What's wrong with using normal std::string concatenation?

340 ↗(On Diff #67482)

Nit, mild preference for "Binary:" over "Binary file:". Also, why are there extra spaces here?

350 ↗(On Diff #67482)

Why are there extra spaces after "Source file:"?

Nit, I'm mildly in favor of shortening "Source file" to "File", but I'll leave it up to you.

571 ↗(On Diff #67482)

clang-format

tools/llvm-cov/SourceCoverageViewText.cpp
241 ↗(On Diff #67482)

This looks really suspicious. I don't think this will look right with large line numbers or large execution counts. I suggest just defining this away for the text view, e.g: SourceCoverageViewText::renderTableHeader(raw_ostream &, unsigned).

MaggieYi marked 3 inline comments as done.Aug 11 2016, 8:28 AM

Many thanks for your comments. As I will be busy on other work in next several days, I will update the patch following your comments once I get those on track. Thanks for the patience.

MaggieYi updated this revision to Diff 69027.Aug 23 2016, 12:40 PM
MaggieYi marked 7 inline comments as done.

Hi Vedant,

I have updated the patch following your comments.

Thanks for your patience.

MaggieYi added inline comments.Aug 23 2016, 12:41 PM
tools/llvm-cov/CodeCoverage.cpp
470 ↗(On Diff #67482)

Ok, I will create a separate patch for this issue.

633 ↗(On Diff #67482)

I checked the generated report on Linux and Windows. The created time formats are the same, which is “Created: 16-01-07 13:12”. If using “MTime.st()”, the created times will be “Created: 16-01-07 13:12:48.216418700” on both Linux and Windows. Since the time string is quite long, I change the code to output the full time string if it can’t find the “:”. What do you think?

tools/llvm-cov/SourceCoverageViewHTML.cpp
340 ↗(On Diff #67482)

Removed the extra spaces.

350 ↗(On Diff #67482)

Again, removed the extra space. How about using “Source”?

vsk edited edge metadata.Aug 23 2016, 1:29 PM

This is looking really close. Just a few minor comments --

tools/llvm-cov/CodeCoverage.cpp
631 ↗(On Diff #69027)

This takes a reference to a temporary, which might make asan upset. Please use std::string.

634 ↗(On Diff #69027)

Sgtm, thanks.

tools/llvm-cov/SourceCoverageView.h
242 ↗(On Diff #69027)

Please update the doxygen here to mention that the function also prints \p CellText.

243 ↗(On Diff #69027)

Missing parameter name.

tools/llvm-cov/SourceCoverageViewHTML.cpp
343 ↗(On Diff #69027)

Nit, this copies the source name twice. How about 'SourceFile = "Function" or "Source"; SourceFile += getSourceName();'?

347 ↗(On Diff #69027)

Sgtm.

349 ↗(On Diff #69027)

Nit, how about 'escape("Binary: " + getOptions().ObjectFilename)'?

552 ↗(On Diff #69027)

Does this need to be gated with a check to hasProjectTitle()?

559 ↗(On Diff #69027)

Ditto, do we need a check here for hasCreatedTime()?

tools/llvm-cov/SourceCoverageViewHTML.h
73 ↗(On Diff #69027)

Missing parameter name.

tools/llvm-cov/SourceCoverageViewText.cpp
225 ↗(On Diff #69027)

Ditto, do we need checks here for hasProjectTitle() / hasCreatedTime()?

MaggieYi updated this revision to Diff 69055.Aug 23 2016, 4:35 PM
MaggieYi edited edge metadata.
MaggieYi marked 9 inline comments as done.

I have fixed the issues, Thanks.

tools/llvm-cov/SourceCoverageViewHTML.cpp
552 ↗(On Diff #69027)

Yes, fixed it. Thanks.

559 ↗(On Diff #69027)

Yes, fixed it. Thanks.

tools/llvm-cov/SourceCoverageViewText.cpp
225 ↗(On Diff #69027)

Yes, fixed it. Thanks.

vsk accepted this revision.Aug 23 2016, 4:40 PM
vsk edited edge metadata.

Thank you! Lgtm.

This revision is now accepted and ready to land.Aug 23 2016, 4:40 PM
This revision was automatically updated to reflect the committed changes.