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

Supplements of guideline of compilation on macOS with make and clean up some code #334

Merged
merged 6 commits into from
Jul 20, 2021

Conversation

LykxSassinator
Copy link
Contributor

I have tried to compile the KVRocks on MacOS, with 11.4 Big Sur, finding that there have missed some tools for compilation on the first time. So i have make the supplements in the README.md.
Unfortunately, it seems that there still exists some limitations and errs while compiling KVRocks on MacOS by make with scripts automatically, which should be manually interfered with. Therefore, currently, it's more recommended to use cmake on MacOS or compiling it on other Linux systems.

@LykxSassinator LykxSassinator changed the title Supplements of guideline of compilation on MacOS with make. Supplements of guideline of compilation on MacOS with make and Code clean. Jul 18, 2021
@LykxSassinator
Copy link
Contributor Author

It's my unpleasable push of two separatable commits. The second commit is used for making several tiny improvements by converting unnecessary usages of Copy Constructor with the appropriate movement ops.

@git-hulk
Copy link
Member

git-hulk commented Jul 18, 2021

cool, thanks for your contribution. Can u help to fix issues found by cpplint?

src/redis_db.cc:468:  Extra space for operator  ++;  [whitespace/operators] [4]
src/redis_hash.cc:178:  Add #include <utility> for move  [build/include_what_you_use] [4]

@git-hulk git-hulk requested review from ShooterIT and git-hulk July 18, 2021 10:19
@git-hulk git-hulk added A-docs area docs enhancement type enhancement labels Jul 18, 2021
src/redis_db.cc Outdated Show resolved Hide resolved
src/cron.cc Outdated Show resolved Hide resolved
LykxSassinator and others added 2 commits July 19, 2021 14:46
As recommended, apply the suggestions from code review.

Co-authored-by: Wang Yuan <wangyuancode@163.com>
Here, I reovke unnecessary and unrecommend std::move operation on the `st` object.
Copy link
Contributor Author

@LykxSassinator LykxSassinator left a comment

Choose a reason for hiding this comment

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

All codes have been reviewed and checked again.
And It's my pleasure to make contributions to KVRocks.

README.md Show resolved Hide resolved
@ShooterIT
Copy link
Member

cpplint error https://github.com/KvrocksLabs/kvrocks/runs/3101400846#step:4:721 please add necessary header files.

Copy link
Contributor Author

@LykxSassinator LykxSassinator left a comment

Choose a reason for hiding this comment

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

Done.

README.md Show resolved Hide resolved
Copy link
Contributor Author

@LykxSassinator LykxSassinator left a comment

Choose a reason for hiding this comment

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

The error checked by cpplint has been fixed.

Copy link
Member

@git-hulk git-hulk left a comment

Choose a reason for hiding this comment

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

LGTM

@ShooterIT ShooterIT changed the title Supplements of guideline of compilation on MacOS with make and Code clean. Supplements of guideline of compilation on macOS with make and clean up code Jul 20, 2021
@ShooterIT ShooterIT changed the title Supplements of guideline of compilation on macOS with make and clean up code Supplements of guideline of compilation on macOS with make and clean up some code Jul 20, 2021
@ShooterIT ShooterIT merged commit cfc1e07 into apache:unstable Jul 20, 2021
ShooterIT pushed a commit to ShooterIT/kvrocks that referenced this pull request Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs area docs enhancement type enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants