-
Notifications
You must be signed in to change notification settings - Fork 12.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[llvm-cov][CoverageView] minor fix/improvement to HTML and text coverage output #80952
Conversation
✅ With the latest revision this PR passed the C/C++ code formatter. |
133ddfb
to
0a07a74
Compare
I can confirm these commits are breaking two tests, and specifically the HTML part:
I'll investigate and update tests if that's the reason for failure. |
0a07a74
to
c5031da
Compare
The tests should have be fixed. Basically change the expected HTML output accordingly (due to commit |
…bViews() Otherwise the generated HTML will place elements unpredictably. Most notably, the MC/DC view boxes are often placed at the page top instead of inline next to the actual code.
Highlight the selected line. One major benefit is, if the page is relatively short, the line pointed by #Ln in URL may not be on top, and this highlight can help quickly locate the line.
…ithin MC/DC views Or it would conflict with the link at actual lines and jumping via URLs containing #Ln would fail
c5031da
to
30a167d
Compare
LGTM -- thanks for catching these. It would be great if you could backport this to the 18.x branch. |
Yes I'm happy to. I don't know the procedures well. To my preliminary understanding: first let's wait for this one to get merged into |
Yes, I think you can follow the steps outlined here: https://llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches |
A gentle bump for more comments. (Which members are the best to include?) By the way, @evodius96 do you have plans of updating this page https://llvm.org/docs/CoverageMappingFormat.html ? We probably want to include the changes since version 6. I can also do a draft for that. Also could you please take a look at #80531? Much appreciated! |
Your fix is minor and straightforward; I don't see the need for additional reviewers, but it's not a problem.
You're right -- the document needs to be updated for version 7. I did add information about the Decision mapping region in the Mapping Region section, but the discussion of what changed between version 6 and 7 also needs to mention that. If you want to draft that change, that's OK with me.
I can take a look, but it does look like you have the right reviewers on it. |
…age output (llvm#80952) 1. add the missing condition for MC/DC in hasSubViews() 2. add style for selected line 3. remove name="Ln" attribute in the link within MC/DC views 4. remove color for \n (cherry picked from commit 0bf4f82)
@evodius96 Comments are appreciated!