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

Fix compile warnings in kvrocks #653

Merged
merged 8 commits into from
Jun 24, 2022
Merged

Conversation

jackwener
Copy link
Member

@jackwener jackwener commented Jun 23, 2022

close 639

fix problem found by tsan.

Thanks @PragmaTwice .

Fix warning.
Add more gitignore (I think them also are needed by others.)

@jackwener jackwener force-pushed the tsan branch 2 times, most recently from 730032d to 46fe25a Compare June 23, 2022 10:03
@jackwener
Copy link
Member Author

raiseError() is a little strange, it is an unreturned function.

Reasonably, we should return void. But it will influent origin logic.

So, I don't handle it. It's a future ticket.

@jackwener jackwener changed the title fix: fix problem found by tsan [WIP] fix: fix problem found by tsan Jun 23, 2022
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

IIRC we disable TSan checker due to several problem it detected but not be resolved.

@PragmaTwice can we re-introduce TSan checker after this fix? If so, we should include the CI yaml changes into this patch.

@jackwener
Copy link
Member Author

jackwener commented Jun 23, 2022

I have tried four time but failed reproduce it.😂. (It may be related with OS)

Env:

  • mbp m1
  • clang version 13.0.1

investigate this problem later (other PR), this PR just fix warning.

@jackwener jackwener changed the title [WIP] fix: fix problem found by tsan fix: fix the warning. Jun 23, 2022
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated
Comment on lines 45 to 46
kvrocks
kvrocks2redis
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
kvrocks
kvrocks2redis

I think these files can be remove since these are in build directories.

@jackwener jackwener force-pushed the tsan branch 3 times, most recently from 21db8cf to 9174063 Compare June 23, 2022 12:20
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

@PragmaTwice PragmaTwice changed the title fix: fix the warning. Fix compile warnings in kvrocks Jun 23, 2022
Copy link
Member

@PragmaTwice PragmaTwice left a comment

Choose a reason for hiding this comment

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

LGTM

@PragmaTwice
Copy link
Member

Thanks for your contribution! Merging...

@PragmaTwice PragmaTwice merged commit 7976e56 into apache:unstable Jun 24, 2022
@jackwener jackwener deleted the tsan branch July 1, 2022 12:31
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.

5 participants