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

Cleanup includes in dbformat.h #8930

Closed
wants to merge 4 commits into from
Closed

Conversation

mrambacher
Copy link
Contributor

This header file was including everything and the kitchen sink when it did not need to. This resulted in many places including this header when they needed other pieces instead.

Cleaned up this header to only include what was needed and fixed up the remaining code to include what was now missing.

Hopefully, this sort of code hygiene cleanup will speed up the builds...

Do not include files that are not directly needed by this header.  Move the includes into the source files requiring them.  Do not include dbformat in places it is not required.
Copy link
Contributor

@pdillinger pdillinger left a comment

Choose a reason for hiding this comment

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

Forward declarations are not generally recommended under Google C++ style, but I don't think we're going to run into issues in these cases.

@@ -5,7 +5,10 @@

#pragma once

#include <cinttypes>
Copy link
Contributor

Choose a reason for hiding this comment

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

This header includes things beyond <cstdint> that are not generally needed. Recommend changing all cinttypes to cstdint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated for this change but there are almost 100 other places that include cinttypes. Will leave that exercise to another day...

Copy link
Contributor

Choose a reason for hiding this comment

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

I recently tried changing one and the file used PRIu64 so we can't replace it everywhere.

@facebook-github-bot
Copy link
Contributor

@mrambacher has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@mrambacher has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@mrambacher has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

yoori pushed a commit to yoori/rocksdb that referenced this pull request Nov 26, 2023
Summary:
This header file was including everything and the kitchen sink when it did not need to.  This resulted in many places including this header when they needed other pieces instead.

Cleaned up this header to only include what was needed and fixed up the remaining code to include what was now missing.

Hopefully, this sort of code hygiene cleanup will speed up the builds...

Pull Request resolved: facebook/rocksdb#8930

Reviewed By: pdillinger

Differential Revision: D31142788

Pulled By: mrambacher

fbshipit-source-id: 6b45de3f300750c79f751f6227dece9cfd44085d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants