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.

Sign up for GitHub

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

Jump to bottom

Minor RocksJava Java code cosmetics #9204

Closed
wants to merge 6 commits into from
Closed

Minor RocksJava Java code cosmetics #9204

wants to merge 6 commits into from

Conversation

stefan-zobel
Copy link
Contributor

Specifically:

  • unused imports
  • code formatting
  • typos in comments
  • unnecessary casts
  • missing default label in switch statement
  • explicit use of long literals in multiplication
  • use generics where possible without backward compatibility risk

@stefan-zobel
minor code cosmetics
8fb6636 
Specifically:
- unused imports
- code formatting
- typos in comments
- unnecessary casts
- missing default label in switch statement
- explicit use of long literals in multiplication
- use generics where possible without backward compatibility risk
@stefan-zobel
Copy link
Contributor Author

Sorry, I really don't get what the format check is complaining about.

@adamretter
Copy link
Collaborator

@stefan-zobel You can just run make format and it will prompt you to reformat any issues for you and allow you to append a commit

adamretter
adamretter reviewed Nov 23, 2021
View reviewed changes
java/src/main/java/org/rocksdb/MutableColumnFamilyOptionsInterface.java Outdated
@@ -26,7 +26,7 @@
* @throws java.lang.IllegalArgumentException thrown on 32-Bit platforms
* while overflowing the underlying platform specific value.
*/
MutableColumnFamilyOptionsInterface setWriteBufferSize(long writeBufferSize);
MutableColumnFamilyOptionsInterface<T> setWriteBufferSize(long writeBufferSize);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just T as per the other functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adamretter That sounds sensible. It just didn't occur to me.

@stefan-zobel
Copy link
Contributor Author

@stefan-zobel You can just run make format and it will prompt you to reformat any issues for you and allow you to append a commit

build_tools/format-diff.sh
Checking format of uncommitted changes...
Nothing needs to be reformatted!

@stefan-zobel
Copy link
Contributor Author

I'll try again ...

@stefan-zobel stefan-zobel reopened this Nov 23, 2021
@stefan-zobel
Copy link
Contributor Author

stefan-zobel commented Nov 23, 2021 •
edited
Loading

To no avail. Still format error and make format says nothing needs to be reformatted.
I'm withdrawing this PR.

@adamretter
Copy link
Collaborator

@stefan-zobel Could it be that you make an older version of the tools used by make format installed on your machine? The output from make format in CI is fairly clear - https://github.com/facebook/rocksdb/runs/4305435864?check_suite_focus=true#step:10:1

@stefan-zobel
Copy link
Contributor Author

stefan-zobel commented Nov 24, 2021 •
edited
Loading

@adamretter
I was running it on Manjaro and used clang-format-static-bin from the Arch User Repository. That should be very current.
My guess is the problem is that it is only looking at the uncomitted changes as its output says. I had to manually edit each already committed file to make it do anything.

@stefan-zobel
Copy link
Contributor Author

Reopening on more time

@stefan-zobel stefan-zobel reopened this Nov 24, 2021
@adamretter
Copy link
Collaborator

is only looking at the uncomitted changes as its output says. I had to manually edit each already committed file to make it do anything.

That hasn't been my experience, my experience is that it considers all my commits in a branch and offers to either amend the formats to the last commit or leave them staged so that I can create just a formatting commit

@stefan-zobel
Copy link
Contributor Author

That hasn't been my experience, my experience is that it considers all my commits in a branch and offers to either amend the formats to the last commit or leave them staged so that I can create just a formatting commit

@adamretter
Interesting. Yes, it's sort of weird.

And it gets even more confusing when the format check nags about formatting errors in parts of the code where the formatting hasn't changed at all.
For example, I didn't touch the formatting of the comment in CompactRangeOptions (my only change was to replace the 4 letters "neum" by the 4 letters "enum") and something similar holds for the format of the TransactionalDB interface declaration.
It took me a while to realize that the check wants me to change the formatting anyway.

@adamretter
Copy link
Collaborator

adamretter commented Nov 24, 2021 •
edited
Loading

I didn't touch the formatting of the comment

Perhaps it is the case that somehow that code was committed before the formatting checks were introduced, now that you touch that area of the code the formatter complains within the context of your non-format related change?

@stefan-zobel
Copy link
Contributor Author

stefan-zobel commented Nov 24, 2021 •
edited
Loading

I didn't touch the formatting of the comment

Perhaps it is the case that somehow that code was committed before the formatting checks were introduced, now that you touch that area of the code the formatter complains within the context of your non-format related change?

Sure. There is no other sensible explanation. By chance I've noticed one other place that wouldn't survive the format check if someone would change a single letter. It's just that I didn't expect that behavior.

@adamretter
Copy link
Collaborator

@stefan-zobel Did you mean to close this?

@stefan-zobel
Copy link
Contributor Author

@stefan-zobel Did you mean to close this?

@adamretter

Yes, why leave it open if no one seems to be interested?

@adamretter
Copy link
Collaborator

adamretter commented Dec 8, 2021 •
edited
Loading

@stefan-zobel I don't think it is that no one is interested (there is clearly interest from myself above).
In the RocksDB project it can take some time to get code merged (I don't have merge rights). Let me see... @jay-zhuang @mrambacher Can you get this one merged please?

@adamretter adamretter reopened this Dec 8, 2021
@facebook-github-bot
Copy link
Contributor

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

jay-zhuang
jay-zhuang approved these changes Dec 8, 2021
View reviewed changes
Copy link
Contributor

@jay-zhuang jay-zhuang 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

@stefan-zobel has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

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

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamretter adamretter adamretter left review comments

@jay-zhuang jay-zhuang jay-zhuang approved these changes

Assignees
No one assigned
Labels
CLA Signed java-api
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stefan-zobel @adamretter @facebook-github-bot @jay-zhuang