alanphipps marked 13 inline comments as done. alanphipps added a comment. Herald added a subscriber: wenlei.
In D84467#2180421 <https://reviews.llvm.org/D84467#2180421>, @vsk wrote: > I haven't taken a close look at the tests yet, but plan on getting to it soon. > > I'm not sure whether this is something you've already tried, but for this > kind of change, it can be helpful to verify that a stage2 clang self-host > with assertions enabled completes successfully. For coverage specifically, > it's possible to do that by setting '-DLLVM_BUILD_INSTRUMENTED_COVERAGE=On' > during the stage2 cmake step. Thank you for recommending this. I was able to do an instrumented stage2 build successfully, with assertions enabled, and it built fine, and no problems running LIT. I have to say that it was also cool to see branch coverage on the source code I added to support branch coverage! ================ Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:742 + // function's SourceRegions) because it doesn't apply to any other source + // code other than the Condition. + if (CodeGenFunction::isLeafCondition(C)) { ---------------- vsk wrote: > alanphipps wrote: > > vsk wrote: > > > Is there some alternative to pushing and then immediately popping branch > > > regions? Did you try adding the region directly to the function's > > > SourceRegions, and find that there was some issue with the more direct > > > approach? > > The push/pop combo does look strange, and you're right -- I can just add it > > directly to SourceRegions, but the algorithm in popRegions() will do > > additional things like adjust the start/end location if the region spans a > > nested file depth. I think that's still useful to do. Another option is I > > could factor that algorithm out of popRegions() into a new function and > > also call that from createBranchRegions(). > If it's possible/straightforward to factor the region start/end adjustment > code out of popRegions(), that would be really nice. If not, the current > approach looks reasonable. I looked at it again, and I decided to table the refactor for now. The adjustment code is coupled to SourceRegions as is the rest of popRegions(), and it may be necessary to rework the whole of the function to get a nicer refactor. ================ Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:659 +void SourceCoverageViewHTML::renderBranchView(raw_ostream &OS, + BranchView &BRV, ---------------- vsk wrote: > alanphipps wrote: > > vsk wrote: > > > Wdyt of displaying branch-taken counts using tooltips? This would match > > > the way region execution counts are shown, which could be nice, because > > > the information wouldn't interrupt the flow of source code. > > I think using tooltips is a great idea, and I did look into it. > > Ultimately, I decided to defer that work to a future patch to allow for > > some time to research a good way to properly distinguish the branch-taken > > counts from the region counts. > Sounds reasonable. Can the branch coverage text/html output should be opt-in > via a cl::opt until it's finalized? That should make it possible to iterate > on the format without changing what users see. Yep, I added two options: --show-branch-summary, which shows the total branch coverage in the summary report, and this is ON by default. --show-branches={count,percent}, which shows source-level coverage on a given file, which is OFF by default. So it's opt-in. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84467/new/ https://reviews.llvm.org/D84467 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
