Page MenuHomePhabricator

[LLVM-COV] Replace tabs to 2-space indentation in the HTML coverage report.
ClosedPublic

Authored by MaggieYi on Aug 2 2016, 3:26 PM.

Details

Summary

When using llvm-cov.exe to generate the HTML report, the HTML report can look quite different to the source file if it includes tabs. The patch includes:

  1. Use the default tab size is 2 spaces instead of 8 spaces.
  2. Add a command line switch (-tab-size) that allows user to set the tab size.

Diff Detail

Repository
rL LLVM

Event Timeline

MaggieYi updated this revision to Diff 66566.Aug 2 2016, 3:26 PM
MaggieYi retitled this revision from to [LLVM-COV] Replace tabs to 2-space indentation in the HTML coverage report..
MaggieYi updated this object.
MaggieYi added reviewers: vsk, davidxl, bogner, harlanhaskins.
MaggieYi added a subscriber: llvm-commits.
vsk edited edge metadata.Aug 2 2016, 5:28 PM

Mostly looks good. The logic in escape() looks correct.

test/tools/llvm-cov/showTabsHTML.cpp
3 ↗(On Diff #66566)

Does the second RUN line achieve any additional test coverage for this patch? If not, please remove it.

21 ↗(On Diff #66566)

Since this test is just checking whether tabs are expanded properly, please keep the CHECKs on lines {9, 14, 18} and remove the rest. That should make the test easier to maintain.

38 ↗(On Diff #66566)

Ditto, please trim this set of CHECKs.

tools/llvm-cov/CoverageViewOptions.h
37 ↗(On Diff #66566)

Please don't introduce static members -- it should be possible to pass a CoverageViewOptions instance instead.

tools/llvm-cov/SourceCoverageViewHTML.cpp
46 ↗(On Diff #66566)

Capitalize the iterator variable name.

MaggieYi updated this revision to Diff 66654.Aug 3 2016, 6:31 AM
MaggieYi edited edge metadata.

Thanks Vedant, I have updated the patch following your comments.

MaggieYi marked 5 inline comments as done.Aug 3 2016, 6:33 AM

Following Vedant's comments, all issues have been fixed.

vsk accepted this revision.Aug 3 2016, 10:28 AM
vsk edited edge metadata.

Great! LGTM.

This revision is now accepted and ready to land.Aug 3 2016, 10:28 AM
This revision was automatically updated to reflect the committed changes.