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

added initializers to resolve compiler warnings #17

Closed
wants to merge 1 commit into from
Closed

added initializers to resolve compiler warnings #17

wants to merge 1 commit into from

Conversation

simsong
Copy link

@simsong simsong commented Jul 22, 2021

This PR ends these warnings:

thread-pool/thread_pool.hpp: In constructor ‘synced_stream::synced_stream(std::ostream&)’:
thread-pool/thread_pool.hpp:410:5: warning: ‘synced_stream::stream_mutex’ should be initialized in the member initialization list [-Weffc++]
  410 |     synced_stream(std::ostream &_out_stream = std::cout)

@bshoshany
Copy link
Owner

Thanks for the pull request! However, I don't see a reason to make this change. These objects are already default-initialized, so this is redundant. I never use the -Weffc++ flag because I don't find these warnings useful; if you don't explicitly use this flag, then you won't see these warnings.

@bshoshany bshoshany closed this Jul 28, 2021
@simsong
Copy link
Author

simsong commented Jul 28, 2021

You have decided not to accept a pull request that eliminates a compiler warning because you personally do not use the warning flag? With all due respect, that is somewhat short-sighted if your goal is to create a software component that is widely used within the community.

@bshoshany
Copy link
Owner

I just don't think there's anything wrong with the code as it is, it may not be compliant with that specific style guide (Scott Meyers’ Effective C++) but it is 100% correct and perfectly compliant with the C++17 standard.

However, I do understand that this is important to you and possibly to others in the community who are using that style guide, so I will add this change in the next revision.

I release my projects in cumulative updates after editing them locally, so my policy is to not accept pull requests. Instead, I will merge this change into the next release, possibly together with some other changes, and along with a version bump and a corresponding note in README.md and a link to this pull request. Thanks for your contribution :)

@bshoshany bshoshany reopened this Jul 28, 2021
@bshoshany
Copy link
Owner

Update: I incorporated this change in v1.8. Thanks again :)

@bshoshany bshoshany closed this Jul 28, 2021
@simsong
Copy link
Author

simsong commented Jul 28, 2021

Thank you!

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.

2 participants