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

options: Forward declare WriteBufferManager #433

Merged
merged 1 commit into from
Mar 29, 2023
Merged

options: Forward declare WriteBufferManager #433

merged 1 commit into from
Mar 29, 2023

Conversation

AmnonHanuhov
Copy link
Contributor

Fixes #279

@AmnonHanuhov AmnonHanuhov added the bug fix Fixes a known bug label Mar 19, 2023
@AmnonHanuhov AmnonHanuhov self-assigned this Mar 19, 2023
@AmnonHanuhov AmnonHanuhov linked an issue Mar 19, 2023 that may be closed by this pull request
Including instrumented_mutex.h leads to compilation errors #279
Closed
@AmnonHanuhov
Copy link
Contributor Author

@Yuval-Ariel

@udi-speedb
Copy link
Contributor

@AmnonHanuhov - Please add the issue# to the commit message's title. Also, the commit message is not clear enough IMO.

udi-speedb
udi-speedb previously approved these changes Mar 20, 2023
View reviewed changes
@AmnonHanuhov
Copy link
Contributor Author

@AmnonHanuhov - Please add the issue# to the commit message's title. Also, the commit message is not clear enough IMO.

I think the title describes exactly what I did, which is Forward declare WriteBufferManager in the options header. Which info is missing in your opinion?
However, following your comment I added a small explanation to why this is necessary in the description of the commit.

@AmnonHanuhov AmnonHanuhov force-pushed the 279-including-instrumented_mutexh-leads-to-compilation-errors branch from a2d4320 to b80ce89 Compare March 21, 2023 17:35
@AmnonHanuhov
options: Forward declare WriteBufferManager #279
9b2f95c 
Fixes #279
There is a circular dependency between options.h and write_buffer_manager.h.
In order to fix that, I forward declare the WriteBufferManager class in
options.h and include it in options.cc and in other .cc files which
use WriteBufferManager.
@AmnonHanuhov AmnonHanuhov force-pushed the 279-including-instrumented_mutexh-leads-to-compilation-errors branch from b80ce89 to 9b2f95c Compare March 23, 2023 08:42
@udi-speedb
Copy link
Contributor

@AmnonHanuhov - Please have the issue number in parentheses. If it's not clear, please see other commits. Thanks

@Yuval-Ariel
Copy link
Contributor

its fine, i can edit the commit msg when im merging. @udi-speedb , plz approve if there isnt anything else

@udi-speedb
Copy link
Contributor

@Yuval-Ariel Nothing else

@udi-speedb udi-speedb self-requested a review March 28, 2023 08:57
@Yuval-Ariel Yuval-Ariel merged commit ed310b7 into main Mar 29, 2023
@Yuval-Ariel Yuval-Ariel deleted the 279-including-instrumented_mutexh-leads-to-compilation-errors branch May 11, 2023 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@udi-speedb udi-speedb udi-speedb approved these changes

Assignees

@AmnonHanuhov AmnonHanuhov

Labels
bug fix Fixes a known bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Including instrumented_mutex.h leads to compilation errors
3 participants
@AmnonHanuhov @udi-speedb @Yuval-Ariel