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

Kcov zig 4merge (for discussion) #424

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

liyu1981
Copy link

#423
a preview of core changes to kcov for discussion

for zig, it will mark unreachable/@Panic lines as uncover
for c/cpp, need to manual mark with "/* no-cover */"

Here is a screenshot of auto ignoring `unreachable`.

![zig-unreachable-example](https://github.com/liyu1981/kcov/blob/master/nocover/zig-unreachable.png?raw=true)
Copy link
Owner

Choose a reason for hiding this comment

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

All this is nice information, but when "native" kcov supports it, I think it won't be needed (as it's shown in the output anyway).

Copy link
Author

Choose a reason for hiding this comment

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

agree, I will remove this in later cleaned up PR.


after building is done. The binary is at `build/src/kcov`. Copy somewhere and use it.

(Or you can download a copy of Apple Silicon version binary from the release section.)
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can make a release shortly after merge of this, and then the osx homebrew is usually updated quite quickly.

I suppose that can be added to the README, or another documentation file. Since not all distributions are up to date, I don't want to in general recommend installing from there.

Copy link
Author

Choose a reason for hiding this comment

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

same to above

@@ -134,6 +135,12 @@ class CodecovWriter : public WriterBase
IReporter::LineExecutionCount cnt = m_reporter.getLineExecutionCount(file->m_name, n);
std::string hitScore = "0";

std::string& line_str = file->m_lineMap.at(n);
Copy link
Owner

Choose a reason for hiding this comment

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

Kcov now builds with c++17, so you can use auto & for these.

Copy link
Author

Choose a reason for hiding this comment

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

got it.

nTotalExecutedLines += summary.m_executedLines;
nTotalCodeLines += inFilesTotalCodeLines;
nTotalExecutedLines += inFilesTotalExecutedLines;
// nTotalCodeLines += summary.m_lines;
Copy link
Owner

Choose a reason for hiding this comment

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

Remove the comment

Copy link
Author

Choose a reason for hiding this comment

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

will do

summary.m_executedLines);
std::string datum = getIndexHeader(fmt("%s/index.html", de->d_name), name, name, inFilesTotalCodeLines,
inFilesTotalExecutedLines);
// std::string datum = getIndexHeader(fmt("%s/index.html", de->d_name), name, name, summary.m_lines,
Copy link
Owner

Choose a reason for hiding this comment

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

... also here


const std::string& line_str = file->m_lineMap[n];
const std::string& file_name = it->first;
bool should_cover = shouldCover(line_str, file_name);
Copy link
Owner

Choose a reason for hiding this comment

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

I think that all these can probably be moved to reporter.cc instead, to the lineIsCode method. I.e., you could do something like shouldCover(...) && it->second->lineIsCode(...) there, and then you can probably remove all changes to the writers.

At least it's something to try out.

Copy link
Author

Choose a reason for hiding this comment

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

good suggestion, will do

}

FileType getFileExt(const std::string& file_name) {
const std::string zig_ending = ".zig";
Copy link
Owner

Choose a reason for hiding this comment

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

here you could also write this as

constexpr auto relevant_extensions = std::array {
   std::pair{".c", c},
   [...]
};

for (const auto &[ending, extension])
{
    if (endsWith(..., ending)
    {
       return extension;
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants