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

Mark dispatch_init(), init_from_block_range(), m_append() as deprecated #67

Closed
wants to merge 1 commit into from
Closed

Conversation

akr-akari
Copy link
Contributor

I found that some member functions such as dispatch_init, init_from_block_range and m_append should not be public, so I moved them to private.
Thanks!

Copy link
Contributor

@jeking3 jeking3 left a comment

Choose a reason for hiding this comment

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

While I agree with you in principle, given these are already public, any change to their visibility is a breaking change. While nothing else inside boost may call them directly, consuming code might). The proper way to do this would be to deprecate them and get that into release notes so that in a follow-on release one could then actually change their visibility. This gives consumers a warning they may need to make a change and enough time to reasonably make it.

@akr-akari
Copy link
Contributor Author

While I agree with you in principle, given these are already public, any change to their visibility is a breaking change. While nothing else inside boost may call them directly, consuming code might). The proper way to do this would be to deprecate them and get that into release notes so that in a follow-on release one could then actually change their visibility. This gives consumers a warning they may need to make a change and enough time to reasonably make it.

Sorry, I'm still not sure how to mark these member functions as deprecated with warnings. Also these member functions are not in the documentation.

@jeking3
Copy link
Contributor

jeking3 commented Apr 27, 2022

They may not be in the documentation but they are still public. This, along with many other boost libraries have been around for a long time and there's no telling how many consuming applications may call one of those, even if they really shouldn't be. For example someone may have subclassed dynamic_bitset, and protected may be more appropriate. Still, we would need to go through the process of deprecating them I think. I'm going to ask for some advice from @mclow on properly deprecating public methods so that their visibility can be changed.

@glenfe
Copy link
Member

glenfe commented Apr 27, 2022

  • In Boost 1.80.0, announce in documentation and in release notes that these functions will only be provisionally public in Boost 1.81.0 if macros are defined.
  • In Boost 1.81.0 these functions will only be public if some macro like BOOST_DYNAMIC_BITSET_DEPRECATED_PUBLIC is defined. The release notes and documentation will state that in Boost 1.83.0 these functions will be unconditionally private.
  • In Boost 1.83.0 the functions will be unconditionally private.

@jeking3 jeking3 self-requested a review April 27, 2022 17:20
@jeking3 jeking3 mentioned this pull request Apr 27, 2022
@jeking3
Copy link
Contributor

jeking3 commented Apr 27, 2022

We probably want to change the title of this PR and make an actual issue for this because it will span across multiple releases.

@akr-akari akr-akari changed the title Moved some member functions to private Mark dispatch_init(), init_from_block_range(), m_append() as deprecated Apr 27, 2022
@akr-akari
Copy link
Contributor Author

Title has changed.

@jeking3
Copy link
Contributor

jeking3 commented Apr 27, 2022

It seems reasonable that dispatch_init was put there to allow subclasses to override or augment the initialization behavior. Now I'm not so sure it really should be private or move in that direction, maybe protected. I'm starting to wonder if this shouldn't be discussed on the mailing list before we do anything here.

@akr-akari
Copy link
Contributor Author

OK, we'll discuss it.

@akr-akari akr-akari closed this by deleting the head repository Feb 8, 2024
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.

None yet

3 participants