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

Add JSON output option to BHive converter #38

Merged
merged 6 commits into from
Feb 19, 2024

Conversation

boomanaiden154
Copy link
Collaborator

This patch adds a JSON output option to the BHive converter. This makes it significantly easier to implement other scripts down the line that ingest this data. This also cuts down the number of inodes that a large data set will use by a significant amount, which can be a problem on some file systems.

This patch adds a JSON output option to the BHive converter. This makes
it significantly easier to implement other scripts down the line that
ingest this data. This also cuts down the number of inodes that a large
data set will use by a significant amount, which can be a problem on
some file systems.
Copy link
Collaborator

@ondrasej ondrasej left a comment

Choose a reason for hiding this comment

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

One more comment. Feel free to submit merge once it is resolved.


if (file_ec) {
std::cerr << "Failed to open output file: " << json_output_file_path.str()
std::cerr << "Failed to open output file: " << json_output_file_path.c_str()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bit expensive - would just json_output_file_path.str() work?

(maybe SmallString<> should have a std::string_view conversion just like llvm::StringRef does, and then this would work out of the box).

Copy link
Collaborator Author

@boomanaiden154 boomanaiden154 Feb 19, 2024

Choose a reason for hiding this comment

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

I don't think c_str() should be that much more expensive? It pretty much just returns a pointer to the internal buffer here, which is also what ends up happening when converting to a std::string. I've changed it to use static_cast<std::string_view> as .str() just returns a StringRef which doesn't have standard output operators implemented (as they use the LLVM ones). StringRef::str() returns a std::string and doesn't automatically use the std::string_view conversion.

Implementing a std::string_view conversion for llvm::SmallString would make quite a bit of sense and I'll work on getting something up for that.

Let me know if the approach here needs some adjustment and I can open up a new PR fixing it.

@boomanaiden154 boomanaiden154 merged commit 58008ca into google:main Feb 19, 2024
6 checks passed
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.

3 participants