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

Change enum SizeApproximationFlags to enum class #9604

Closed

Conversation

akankshamahajan15
Copy link
Contributor

Summary: Change enum SizeApproximationFlags to enum and class and add
overloaded operators for the transition between enum class and uint8_t

Test Plan: Circle CI jobs

Copy link
Contributor

@riversand963 riversand963 left a comment

Choose a reason for hiding this comment

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

Thanks @akankshamahajan15 for the PR. left a few comments.

include/rocksdb/db.h Outdated Show resolved Hide resolved
include/rocksdb/db.h Outdated Show resolved Hide resolved
include/rocksdb/db.h Outdated Show resolved Hide resolved
include/rocksdb/db.h Outdated Show resolved Hide resolved
Summary: Change enum SizeApproximationFlags to enum and class and add
overloaded operators for the class.

Test Plan: Circle CI jobs
include/rocksdb/db.h Outdated Show resolved Hide resolved
include/rocksdb/db.h Outdated Show resolved Hide resolved
@riversand963
Copy link
Contributor

ah, Java needs to be fixed too

Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
@akankshamahajan15
Copy link
Contributor Author

ah, Java needs to be fixed too

Oh yes. Java build is failing. Do we need to add overloading operators as well in Java?

@ajkr
Copy link
Contributor

ajkr commented Feb 19, 2022

Do we need to add overloading operators as well in Java?

No, they're optional / for convenience.

Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

LGTM

@facebook-github-bot
Copy link
Contributor

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

ajkr added a commit to ajkr/rocksdb that referenced this pull request Feb 20, 2022
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.

4 participants