Skip to content
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

Enable C++ branch coverage when using gcov 8 or later. #18879

Closed
wants to merge 2 commits into from

Conversation

c-mita
Copy link
Member

@c-mita c-mita commented Jul 10, 2023

Branch coverage was disabled due to a gcov bug, but this was fixed for
GCC 8: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84879.

It doesn't make sense to disable all C++ branch coverage for everyone
because of a bug in an old GCC version.

@c-mita c-mita requested a review from lberki as a code owner July 10, 2023 13:21
@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Jul 10, 2023
@Pavank1992 Pavank1992 added the team-Rules-CPP Issues for C++ rules label Jul 10, 2023
@c-mita c-mita force-pushed the cc_branch_coverage branch 3 times, most recently from 33f23ec to ed9ebda Compare July 10, 2023 16:48
@c-mita c-mita requested a review from meisterT July 11, 2023 11:06
@c-mita
Copy link
Member Author

c-mita commented Jul 11, 2023

There's quite a bit of duplication of helper functions in the various coverage related shell tests; I'll have a go at reducing that in a follow up change.

src/test/shell/bazel/bazel_cc_code_coverage_test.sh Outdated Show resolved Hide resolved
# Extract gcov's version: the output of `gcov --version` contains the
# version as a set of major-minor-patch numbers, of which we extract
# the major version.
gcov_major_version=$("${GCOV}" --version | sed -n -E -e 's/^.*\s([0-9]+)\.[0-9]+\.[0-9]+\s?.*$/\1/p')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you using a different mechanism to extract the major version here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's simply replacing what was removed in #18878.

This approach handles using llvm-cov gcov - the tests don't need to worry about that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, when I come to consolidate things, I'll probably use the same approach everywhere (no reason not to).

But right now, I don't want to change things everywhere in the tests...

Branch coverage was disabled due to a gcov bug, but this was fixed for
GCC 8: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84879.

It doesn't make sense to disable all C++ branch coverage for
everyone because of a bug in an old GCC version.

RELNOTES: Enable C++ branch coverage if gcov version is 8 or newer.
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants