Review Request: impl_ptr (pimpl)
After much procrastination I would like to request a formal review for the pimpl library... once and for all :-) (given it's been through a few almost-reviews and discussions)... to get some kind of a resolution one way or the other. :-) On one hand it is embarrassingly simple and no match to a string of late TMP submissions. On the other hand it seems quite handy for ordinary s/w folk (like me) as it formalizes, streamlines and noticeably simplifies (IMO) pimpl-based deployments and makes code-review process quite simple and straightforward. Its benefits are more visible for pimpls with exclusive-ownership proxy-implementation relationships (value-semantics pimpls) as it uses internally an incomplete-type management smart pointer instead of std:unique_ptr. Given that it is policy-based I am hoping others might get interested writing additional policies within that framework such as COW or stack-based. The major oddity/peculiarity (that caused a stir before and Gavin mentioned about a year ago) is that the main class -- impl_ptr -- is declared in the global namespace. That might happen to be a deal breaker but that's the best I could come up with to get it to work. If anyone has a better working solution, I am all ears. That "oddity" is an "implementation detail" and does not affect the user -- from the user side it appears inside boost. Like boost::impl_ptr... Internally to address that we might think of striking a compromise. For ex., instead of the traditional namespace boost { template<> class impl_ptr } have template<> class _boost_impl_ptr_ Again, that is an implementation nit, wrinkle, niggle (IMO anyway). From the outside it all looks kosher -- boost::impl_ptr<...> and such. The code and the docs are at https://github.com/yet-another-user/pimpl <https://github.com/yet-another-user/pimpl>. Due to personal circumstances Glen -- Pimpl's original manager -- had to bail out. So, if anyone kindly steps forward to manage the review, that'd be immensely appreciated. Thank you, Vladimir. P.S. Ron, I am only CCing you so that you'd put it to the schedule... Tnx.
On 15-08-17 02:51, Vladimir Batov via Boost wrote:
That might happen to be a deal breaker but that's the best I could come up with to get it to work. If anyone has a better working solution, I am all ears.
I have the same interest. I'd love to know what the global declaration is "helping" work. I have the intuition that it is not necessary in any way if you properly account for ADL. Can you point at an explanation of _what purpose_ is served, how, by having ::impl_ptr in the global namespace? I'm happy to toy with it and possibly create a PR. In the light of that, it would help if there were tests. Seth
On 08/15/2017 09:57 PM, Seth via Boost wrote:
On 15-08-17 02:51, Vladimir Batov via Boost wrote:
That might happen to be a deal breaker but that's the best I could come up with to get it to work. If anyone has a better working solution, I am all ears. I have the same interest. I'd love to know what the global declaration is "helping" work. I have the intuition that it is not necessary in any way if you properly account for ADL.
Can you point at an explanation of _what purpose_ is served, how, by having ::impl_ptr in the global namespace? I'm happy to toy with it and possibly create a PR.
The thread called "Pimpl Again?" dated June 2016 on this list might be a good start. In short, the reason is the specialization of impl_ptr<Foo>::implementation.
In the light of that, it would help if there were tests.
impl_ptr/test is the test directory -- https://github.com/yet-another-user/pimpl/tree/master/test.
2017-08-15 2:51 GMT+02:00 Vladimir Batov via Boost <boost@lists.boost.org>:
After much procrastination I would like to request a formal review for the pimpl library... once and for all :-) (given it's been through a few almost-reviews and discussions)... to get some kind of a resolution one way or the other. :-)
Hi Vladimir, Is there some summary of changes since the last review? Also, could you remind us what was the reason for not accepting the library the last time? Submission was withdrawn, right? Regards, &rzej;
On 2017-08-16 18:14, Andrzej Krzemienski via Boost wrote:
2017-08-15 2:51 GMT+02:00 Vladimir Batov via Boost <boost@lists.boost.org>: ... Is there some summary of changes since the last review? Also, could you remind us what was the reason for not accepting the library the last time? Submission was withdrawn, right?
It has never been through a proper review. It was scheduled for a review some time ago but I had to withdraw it as it was scheduled right after boost::convert and there was a lot of work after boost::convert conditional acceptance. I was swamped and had to sacrifice, withdraw "pimpl" back then. As for the summary I do not have one. Fundamentally it is the same idea/implementation... that uses/relies on impl_ptr<Foo>::implementation specialization. It is much simpler with C++11. Its interface was brought in-line with C++17 optional, variant, any, i.e. to use in_place. That simplified it even further as many if_enable-based signatures were eliminated. Then, the name changed to "impl_ptr" as "pimpl" turned out too disruptive and controversial :-) -- everyone had their own interpretation of it. Most importantly, it is now policy-based and I implemented 4 policies -- shared ownership, unique exclusive ownership, copied exclusive ownership, stack-based -- which I expect to satisfy the majority of behavioral requests that came up during discussions before. COW policy is on the TODO list.
Hi Vladimir,
On 15. Aug 2017, at 02:51, Vladimir Batov via Boost <boost@lists.boost.org> wrote:
After much procrastination I would like to request a formal review for the pimpl library... once and for all :-) (given it's been through a few almost-reviews and discussions)... to get some kind of a resolution one way or the other. :-)
I looked into the documentation and from a user perspective, impl_ptr looks very nice, I'd love to use it. I also like the documentation so far. It needs some straightening of typos, but it is a light and enjoyable read. Best regards, Hans
Le 16/08/2017 à 17:38, Hans Dembinski via Boost a écrit :
Hi Vladimir,
On 15. Aug 2017, at 02:51, Vladimir Batov via Boost <boost@lists.boost.org> wrote:
After much procrastination I would like to request a formal review for the pimpl library... once and for all :-) (given it's been through a few almost-reviews and discussions)... to get some kind of a resolution one way or the other. :-) I looked into the documentation and from a user perspective, impl_ptr looks very nice, I'd love to use it. I also like the documentation so far. It needs some straightening of typos, but it is a light and enjoyable read.
Hi, I'm missing the reference documentation. Vicente
Vicente, On 2017-08-17 04:49, Vicente J. Botet Escriba via Boost wrote:
... I'm missing the reference documentation.
I just 1) went to https://github.com/yet-another-user/pimpl; 2) clicked on "HTML documentation is available here"; 3) clicked on "References"; 4) got to the references page -- http://yet-another-user.github.io/pimpl/doc/html/impl_ptr/references.html; 5) then tried all reference links on the first "contents+introduction" page; they all took me to that mentioned "references" page. Do you mind trying again and, if it does not work, describe the steps that fail for you? Tnx.
It turns out my Review Request was somewhat premature as the submission procedures changed quite a bit. Now it says that to just have "library added to the review queue" I "need to find at least one member (but preferably several) of the Boost community who is willing to publicly endorse your library for entry into Boost". So, to enter the review queue, I need the lib already reviewed... and approved/endorsed. Hmm, feels like a chicken-n-egg situation... but who am I to judge? So, I was told I 1) should first have a review manager, 2) some candidate review dates, 3) and "seconded" support from the mailing list. So, I guess, it is the time when I humbly request if I can find anyone to step forward for No.1 and No.3 of the list? Please?.. Pretty please?.. Jeez, that surely feels awkward. On 2017-08-15 10:51, Vladimir Batov via Boost wrote:
After much procrastination I would like to request a formal review for the pimpl library... once and for all :-) (given it's been through a few almost-reviews and discussions)... to get some kind of a resolution one way or the other. :-)
On one hand it is embarrassingly simple and no match to a string of late TMP submissions. On the other hand it seems quite handy for ordinary s/w folk (like me) as it formalizes, streamlines and noticeably simplifies (IMO) pimpl-based deployments and makes code-review process quite simple and straightforward. Its benefits are more visible for pimpls with exclusive-ownership proxy-implementation relationships (value-semantics pimpls) as it uses internally an incomplete-type management smart pointer instead of std:unique_ptr.
Given that it is policy-based I am hoping others might get interested writing additional policies within that framework such as COW or stack-based.
The major oddity/peculiarity (that caused a stir before and Gavin mentioned about a year ago) is that the main class -- impl_ptr -- is declared in the global namespace. That might happen to be a deal breaker but that's the best I could come up with to get it to work. If anyone has a better working solution, I am all ears.
That "oddity" is an "implementation detail" and does not affect the user -- from the user side it appears inside boost. Like boost::impl_ptr... Internally to address that we might think of striking a compromise. For ex., instead of the traditional
namespace boost { template<> class impl_ptr }
have
template<> class _boost_impl_ptr_
Again, that is an implementation nit, wrinkle, niggle (IMO anyway). From the outside it all looks kosher -- boost::impl_ptr<...> and such.
The code and the docs are at https://github.com/yet-another-user/pimpl <https://github.com/yet-another-user/pimpl>.
Due to personal circumstances Glen -- Pimpl's original manager -- had to bail out. So, if anyone kindly steps forward to manage the review, that'd be immensely appreciated.
Thank you, Vladimir.
P.S. Ron, I am only CCing you so that you'd put it to the schedule... Tnx.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On 15. Aug 2017, at 02:51, Vladimir Batov via Boost <boost@lists.boost.org> wrote:
Given that it is policy-based I am hoping others might get interested writing additional policies within that framework such as COW or stack-based.
Can you use custom allocators for all policies? If so, then the behavior of the behavior of the "onstack" policy could be achieved by using a stack-based allocator, like the one from Howard, right? https://howardhinnant.github.io/stack_alloc.html I think it is very elegant if one can switch to a stack-based version without huge code-refactoring if the performance need arises (I am paraphrasing your documentation). Best regards, Hans
On 2017-08-18 19:50, Hans Dembinski wrote:
On 15. Aug 2017, at 02:51, Vladimir Batov via Boost <boost@lists.boost.org> wrote:
Given that it is policy-based I am hoping others might get interested writing additional policies within that framework such as COW or stack-based.
Can you use custom allocators for all policies?
Unfortunately no at the moment. It's been high on my TODO list... but given I do not use that functionality I've not had enough incentive implementing it. Still, I actually was reading about allocators right now. :-)
If so, then the behavior of the behavior of the "onstack" policy could be achieved by using a stack-based allocator, like the one from Howard, right?
I am not sure but I'd prefer if was possible. I see that impl_ptr::copied and impl_ptr::onstack are essentially identical... mem. management aside.
https://howardhinnant.github.io/stack_alloc.html
I think it is very elegant if one can switch to a stack-based version without huge code-refactoring if the performance need arises (I am paraphrasing your documentation).
Thank you for the link. Much appreciated. I'll most certainly read as it's what I've been scratching my head about for some time... Had a quick glance. I can be dead wrong but it does not seem (unfortunately) applicable to impl_ptr. It appears to be the standard allocator-usage pattern, i.e. I explicitly create an allocator (and memory arena) and then take from that arena. It's not applicable in the impl_ptr::onstack situation. Again, I'll have to read again.
On 18 August 2017 at 13:47, Vladimir Batov via Boost <boost@lists.boost.org> wrote:
I can be dead wrong but it does not seem (unfortunately) applicable to impl_ptr. It appears to be the standard allocator-usage pattern, i.e. I explicitly create an allocator (and memory arena) and then take from that arena. It's not applicable in the impl_ptr::onstack situation.
Aren't you doing something wrong (in your design) then? You called it: "standard allocator-usage pattern.". Standard is the key word here isn't it, that's what should be used IMHO. degski -- "*Ihre sogenannte Religion wirkt bloß wie ein Opiat reizend, betäubend, Schmerzen aus Schwäche stillend.*" - Novalis 1798
On 2017-08-18 23:05, degski via Boost wrote:
On 18 August 2017 at 13:47, Vladimir Batov via Boost <boost@lists.boost.org> wrote:
I can be dead wrong but it does not seem (unfortunately) applicable to impl_ptr. It appears to be the standard allocator-usage pattern, i.e. I explicitly create an allocator (and memory arena) and then take from that arena. It's not applicable in the impl_ptr::onstack situation.
Aren't you doing something wrong (in your design) then? You called it: "standard allocator-usage pattern.". Standard is the key word here isn't it, that's what should be used IMHO.
It's not about "doing" but rather "not doing". :-) It is an important design decision. Namely, where the allocator deployment is to be declared. One choice is to follow pimpl's philosophy and to deploy it in the implementation, i.e. totally hide it from the user, i.e. the developer decides how his pimpl implementation uses memory. The other choice is the standard (external) allocator deployment, i.e. applying it to the proxy declaration, i.e. giving it to the user. The advantage of the first choice is the same as the main pimpl's advantage -- the developer decides to optimize or not memory usage, when and how. Seems attractive. The advantage of the second choice is that it is the standard allocator deployment with all the perks. Attractive no less. That appears to be a serious design decision with long-term impact and the potential of alienating some users (who are staunchly for one design or the other). Consequently, I was not implementing that hard bit as I personally do not use it. My idea was that, if impl_ptr gets over the "endorsement request" hurdle and gets to the review stage, then I'd bring that design problem to the list and we'd decide one way or the other. If impl_ptr does not get anywhere and is only used in my backyard, then I'll leave it as-is.
https://howardhinnant.github.io/stack_alloc.html I think it is very elegant if one can switch to a stack-based version without huge code-refactoring if the performance need arises (I am paraphrasing your documentation).
Thank you for the link. Much appreciated. I'll most certainly read as it's what I've been scratching my head about for some time... Had a quick glance. I can be dead wrong but it does not seem (unfortunately) applicable to impl_ptr. It appears to be the standard allocator-usage pattern, i.e. I explicitly create an allocator (and memory arena) and then take from that arena. It's not applicable in the impl_ptr::onstack situation. Again, I'll have to read again.
I am not an expert either :). I looked into allocators from the user-perspective so far, and I worked towards making my project, histogram, allocator aware. I like about allocators that they separate memory management from other semantics, like ownership. One can have a shared_ptr or a unique_ptr with memory allocated from a stack or pool, for example. Howard makes the case that there is no need for a special container like boost::small_vector http://www.boost.org/doc/libs/master/doc/html/container/non_standard_contain... if one uses a stack-allocator with the standard vector. That sounds fantastic, so I thought it could also work here? Howard originally had designed another allocator where the arena was internalised. That one would fit better with your policy, I think. It is a downside of Howards stack allocator, that one has to create a separate arena object on the stack before constructing the actual object that uses the memory in the area. I suppose that's what you mean by saying it conflicts with the onstack situation? I googled some more and found his answer on stackoverflow why the arena is separate https://stackoverflow.com/questions/11648202/questions-about-hinnants-stack-... and also the explanation here, lines 23 to 35: https://src.chromium.org/viewvc/chrome/trunk/src/base/containers/stack_conta... I understand from these sources that allocator copies must be able to deallocate memory of each other, which effectively means they have to point to the same arena, hence the area must be external to have a life-time independent of the allocator.
On 2017-08-21 19:13, Hans Dembinski wrote:
https://howardhinnant.github.io/stack_alloc.html I think it is very elegant if one can switch to a stack-based version without huge code-refactoring if the performance need arises (I am paraphrasing your documentation).
Thank you for the link. Much appreciated. I'll most certainly read as it's what I've been scratching my head about for some time... Had a quick glance. I can be dead wrong but it does not seem (unfortunately) applicable to impl_ptr. It appears to be the standard allocator-usage pattern, i.e. I explicitly create an allocator (and memory arena) and then take from that arena. It's not applicable in the impl_ptr::onstack situation. Again, I'll have to read again.
I am not an expert either :). I looked into allocators from the user-perspective so far, and I worked towards making my project, histogram, allocator aware. I like about allocators that they separate memory management from other semantics, like ownership. One can have a shared_ptr or a unique_ptr with memory allocated from a stack or pool, for example. Howard makes the case that there is no need for a special container like boost::small_vector
http://www.boost.org/doc/libs/master/doc/html/container/non_standard_contain...
if one uses a stack-allocator with the standard vector. That sounds fantastic, so I thought it could also work here?
Howard originally had designed another allocator where the arena was internalised. That one would fit better with your policy, I think. It is a downside of Howards stack allocator, that one has to create a separate arena object on the stack before constructing the actual object that uses the memory in the area. I suppose that's what you mean by saying it conflicts with the onstack situation?
I googled some more and found his answer on stackoverflow why the arena is separate
https://stackoverflow.com/questions/11648202/questions-about-hinnants-stack-...
and also the explanation here, lines 23 to 35: https://src.chromium.org/viewvc/chrome/trunk/src/base/containers/stack_conta...
I understand from these sources that allocator copies must be able to deallocate memory of each other, which effectively means they have to point to the same arena, hence the area must be external to have a life-time independent of the allocator.
Thank you for the input and the links... and the nice allocator-usage-related description. Much appreciated. And, yes, your hunch that, if allocators work with std::vector, they should work with impl_ptr is correct (with caveats related to different std::vector and impl_ptr deployments patterns). I went down that path and introduced allocators... impl_ptr::onstack included... internally. That seemed easier to maintain and to use.
On 15/08/2017 12:51, Vladimir Batov wrote:
After much procrastination I would like to request a formal review for the pimpl library... once and for all :-) (given it's been through a few almost-reviews and discussions)... to get some kind of a resolution one way or the other. :-) [...] The code and the docs are at https://github.com/yet-another-user/pimpl <https://github.com/yet-another-user/pimpl>.
On a quick pass through the docs, http://yet-another-user.github.io/pimpl/doc/html/impl_ptr/behind_the_interfa... suggests that Book::operator== be implemented using (*this)->title etc. Given the implementation on the value-semantics page (http://yet-another-user.github.io/pimpl/doc/html/impl_ptr/pimpl_with_exclusi...), at first glance this *looks* like you're comparing the Book::title() member pointers, which is obviously wrong. Now, I know from the other comment on the former page that this is actually an idiom for accessing the implementation's member and not the public member, but this seems too confusing and error-prone, especially given the common names. I would recommend not overloading operator-> and operator* for impl access and instead requiring that this be more explicit, such as impl().title (or as the page also suggests impl.title, but after a glance at the implementation I didn't see how that would work). From http://yet-another-user.github.io/pimpl/doc/html/impl_ptr/construction_and_i..., perhaps consider implicit construction from nullptr_t as another method to create an empty instance; that might be more natural for people. Also, the generated reference docs seem a bit useless -- http://yet-another-user.github.io/pimpl/doc/html/reference.html shows only two types, including _internal_impl_ptr, which is not itself documented and just 404s.
Gavin, Thank you for the input. On 2017-08-21 18:08, Gavin Lambert via Boost wrote:
On 15/08/2017 12:51, Vladimir Batov wrote: ... On a quick pass through the docs, http://yet-another-user.github.io/pimpl/doc/html/impl_ptr/behind_the_interfa... suggests that Book::operator== be implemented using (*this)->title etc. ... I would recommend not overloading operator-> and operator* for impl access and instead requiring that this be more explicit, such as impl().title (or as the page also suggests impl.title, but after a glance at the implementation I didn't see how that would work).
Yes, I hear you. I've been using the syntax for quite some time and I am used to it. Certainly, there is nothing wrong in additionally providing impl() or something of that sort.
From http://yet-another-user.github.io/pimpl/doc/html/impl_ptr/construction_and_i..., perhaps consider implicit construction from nullptr_t as another method to create an empty instance; that might be more natural for people.
Good point. I'll add it.
Also, the generated reference docs seem a bit useless -- http://yet-another-user.github.io/pimpl/doc/html/reference.html shows only two types, including _internal_impl_ptr, which is not itself documented and just 404s.
Indeed. Just added it today and still fighting Doxygen... it refuses to document the "detail" namespace... no matter what I try.
On 2017-08-21 18:08, Gavin Lambert via Boost wrote:
... Also, the generated reference docs seem a bit useless -- http://yet-another-user.github.io/pimpl/doc/html/reference.html shows only two types, including _internal_impl_ptr, which is not itself documented and just 404s.
Fixed the 404. It turns out Doxygen was generating "_internal_impl_ptr.html" page for the "_internal_impl_ptr" class and that page was displayed properly locally. However, when copied onto Github, it resulted in 404 as Gihub does not show (ignore?) pages starting with underscore. Now that I renamed "_internal_impl_ptr" to "boost_impl_ptr_detail", the page shows.
2017-08-15 2:51 GMT+02:00 Vladimir Batov via Boost <boost@lists.boost.org>:
After much procrastination I would like to request a formal review for the pimpl library... once and for all :-) (given it's been through a few almost-reviews and discussions)... to get some kind of a resolution one way or the other. :-)
The code and the docs are at https://github.com/yet-another-user/pimpl < https://github.com/yet-another-user/pimpl>.
I find it disturbing that the first example describes using shared_ptr-s and defensive null checks. Boost has always been promoting the STL-like value-semantic style of programming, and this usage of shared_ptr's looks counter to the trend. When I am defining type Book -- unless I have to hide implementation, I will just put the members directly in order to achieve value semantics, in particular, I want to guarantee the following property: Book b1 {"title", "author"}; Book b2 = b1; Book b3 = b1; modify(b1); assert (b2 == b3); assert (b2 != b1); The shared_ptr does no seem to guarantee the last assertion. Also, there are these additional members testing if pimpl is null, which appear suspicious to me. How can it be null? Do I have to implement the default constructor now? Does this also mean than in my types that I want to pimpl-ize I cannot use operator bool for my own business-logic-related purpose, because it is already taken by the implementation of boost::pimpl? There may be good use cases for flyweight-like implementation, but it should not necessarily be the first example we see in the docs. Regards, &rzej;
On 08/21/2017 10:38 PM, Andrzej Krzemienski via Boost wrote:
After much procrastination I would like to request a formal review for the pimpl library... I find it disturbing that the first example describes using shared_ptr-s and defensive null checks. Boost has always been
2017-08-15 2:51 GMT+02:00 Vladimir Batov via Boost <boost@lists.boost.org>: promoting the STL-like value-semantic style of programming, and this usage of shared_ptr's looks counter to the trend. When I am defining type Book -- unless I have to hide implementation, I will just put the members directly in order to achieve value semantics, in particular, I want to guarantee the following property: Book b1 {"title", "author"}; Book b2 = b1; Book b3 = b1; modify(b1); assert (b2 == b3); assert (b2 != b1); The shared_ptr does no seem to guarantee the last assertion. Also, there are these additional members testing if pimpl is null, which appear suspicious to me. How can it be null? Do I have to implement the default constructor now? Does this also mean than in my types that I want to pimpl-ize I cannot use operator bool for my own business-logic-related purpose, because it is already taken by the implementation of boost::pimpl? There may be good use cases for flyweight-like implementation, but it should not necessarily be the first example we see in the docs.
Andrzej, To be honest I am somewhat surprised... in part by the fact that you find the first example no less than "disturbing". I do hear your view and I find it is a perfectly valid view. Still, I am sure you've been around the bush for long enough to know that there are as many views/opinions/preferences as there are people. Then different projects have different requirements. So, there should not be anything "disturbing" if you see someone doing something his way rather than yours to satisfy the requirements of their project rather than yours. Don't you agree? Then you go on to explain that the shared-properties pimpl does not suit your requirements and to describe your potential confusion (and even frustration) with such pimpls... Where all that is coming from? No one is forcing you to use the shared-properties pimpl, right? Exclusive-ownership pimpls are equal-rights' citizens in the implementation. They are equally presented in the documentation. What is there to be "disturbed" by? Indeed, in the documentation I do mention/describe the shared-properties pimpl first... because it is easier to introduce the Pimpl concept using std::shared_ptr. Different people have different skill/knowledge levels, different expectations, different priorities, different reading styles. There is no way to satisfy them all. Please do not be "disturbed" if you do not see things done exactly you'd do that.
Also, there are these additional members testing if pimpl is null, which appear suspicious to me. How can it be null?
std::optional has relational operators and can be "null", invalid. std::shared_ptr() has relational operators and can be "null", invalid. unique_ptr has relational operators and can be "null", invalid. The class I proposed -- impl_ptr -- has relational operators and can be "null", invalid. What is so suspicious about it? The name itself -- impl_ptr -- highlights the fact that it is a smart pointer, a handle/body construct. So, the handle can be associated with a body... or the handle might not have a body, i.e. "invalid", "null".
Do I have to implement the default constructor now?
No, you do not. Your frustration seems palpable. I fail to understand how possibly I could cause that.
Does this also mean than in my types that I want to pimpl-ize I cannot use operator bool for my own business-logic-related purpose, because it is already taken by the implementation of boost::pimpl?
No, it does not mean anything like you describe... and it not "taken". It is provided. If you want your "own business-logic", you implement it and you use it. There is nothing stopping you from doing that, right? Unless I missed, mis-understood something.
2017-08-22 2:34 GMT+02:00 Vladimir Batov via Boost <boost@lists.boost.org>:
On 08/21/2017 10:38 PM, Andrzej Krzemienski via Boost wrote:
2017-08-15 2:51 GMT+02:00 Vladimir Batov via Boost <boost@lists.boost.org
:
After much procrastination I would like to request a formal review for the pimpl library...
I find it disturbing that the first example describes using shared_ptr-s and defensive null checks. Boost has always been promoting the STL-like value-semantic style of programming, and this usage of shared_ptr's looks counter to the trend. When I am defining type Book -- unless I have to hide implementation, I will just put the members directly in order to achieve value semantics, in particular, I want to guarantee the following property: Book b1 {"title", "author"}; Book b2 = b1; Book b3 = b1; modify(b1); assert (b2 == b3); assert (b2 != b1); The shared_ptr does no seem to guarantee the last assertion. Also, there are these additional members testing if pimpl is null, which appear suspicious to me. How can it be null? Do I have to implement the default constructor now? Does this also mean than in my types that I want to pimpl-ize I cannot use operator bool for my own business-logic-related purpose, because it is already taken by the implementation of boost::pimpl? There may be good use cases for flyweight-like implementation, but it should not necessarily be the first example we see in the docs.
Andrzej,
To be honest I am somewhat surprised... in part by the fact that you find the first example no less than "disturbing". I do hear your view and I find it is a perfectly valid view. Still, I am sure you've been around the bush for long enough to know that there are as many views/opinions/preferences as there are people. Then different projects have different requirements. So, there should not be anything "disturbing" if you see someone doing something his way rather than yours to satisfy the requirements of their project rather than yours. Don't you agree?
Then you go on to explain that the shared-properties pimpl does not suit your requirements and to describe your potential confusion (and even frustration) with such pimpls... Where all that is coming from? No one is forcing you to use the shared-properties pimpl, right? Exclusive-ownership pimpls are equal-rights' citizens in the implementation. They are equally presented in the documentation. What is there to be "disturbed" by? Indeed, in the documentation I do mention/describe the shared-properties pimpl first... because it is easier to introduce the Pimpl concept using std::shared_ptr. Different people have different skill/knowledge levels, different expectations, different priorities, different reading styles. There is no way to satisfy them all. Please do not be "disturbed" if you do not see things done exactly you'd do that.
Part of my complaint is indeed coming from my personal judgement of different implementation strategies. The other part is me giving you a feedback from the "user experience" of a potential user of your library. I am an impatient programmer who is facing alternative: implement pimpl manually or use a third-party library, there might be like 100 libraries for this purpose with different quality, and I have very little time to asses if it is better to use one of them, or to write my own. Having encountered a library I need to be able to answer a couple of questions quickly, like, is this library worth investing time in even reading the full documentation. So, one of the first questions I would like being answered is. Is the author of this library aware of the present programming styles, like value semantics, zombie-object avoidance, or is (s)he confined to these older OO-like techniques, where everything is pointers and garbage-collected (or shared_ptr-collected) memory with null pointers causing bugs? I cannot figure this out from the docs of Boost.Pimpl quickly. Ultimately, I can figue it out from studying the entire docs, or the implementations, or private conversation with you. But: I cannot figure it out *quickly* from the *initial pages* of the docs. First page: no information -- I know what pimpl is already. Second page: "the basics" -- I do not want the basics. I want answers, preferably in form of a short example. There is a word "unique-ownership" highlighted, so this is something, but still some questions unanswered: will I have null pointers/null states all around? Third page: I get an example of something I would not like to get. I give you that it your order of presenting policies is no worse than my expected order of policies, but I can see these two functions in the interface for testing for the null state. Note that separating interface from implementation has nothing to do with offering an additional null state. The two things are orthogonal; it is just that it is easy to provide a null state when you are implementing a handle anyway -- but they are different things, and they distract attention. It is not clear from the first glimpse at the example whether I am forced to have the null state or whether I can opt-in to have it. By that time I will have exhausted the time I was willing to devote to studying the library usefulness, and would move to the next library or to implementing my own pimpl. Note that my remarks are to the structure of the initial pages of the docs: not to your design choices.
Also, there are these additional members testing if pimpl is null, which appear suspicious to me. How can it be null?
std::optional has relational operators and can be "null", invalid. std::shared_ptr() has relational operators and can be "null", invalid. unique_ptr has relational operators and can be "null", invalid. The class I proposed -- impl_ptr -- has relational operators and can be "null", invalid. What is so suspicious about it? The name itself -- impl_ptr -- highlights the fact that it is a smart pointer, a handle/body construct. So, the handle can be associated with a body... or the handle might not have a body, i.e. "invalid", "null".
Of optional, I expect a null state. This is what optional is for and this is reflected in its type. Of shared_ptr I do not necessarily expect a null state, and I treat it as necessary evil, and long for types with shared semantics but without null state which is causing many bugs in my programs I work with. Of "pimpl" library I expect separation of interface and implementation: not a null state. If you provide it, I would expect a clear indication that it is an opt-in feature.
Do I have to implement the default constructor now?
No, you do not. Your frustration seems palpable. I fail to understand how possibly I could cause that.
It is not you causing it. Maybe I am just a nerd. I have bad experience with having to spend lot of time in my company fixing bugs after people who overused OO and shared_ptrs. I see now the docs of Boost.Pimpl unnecessarily promoting these. Doing flyweighs requires taking care of oher things like using shared_ptr<const T> rather than shared_ptr<mutable T>, which the docs do not cover.
Does this also mean than in my types that I want to pimpl-ize I cannot use operator bool for my own business-logic-related purpose, because it is already taken by the implementation of boost::pimpl?
No, it does not mean anything like you describe... and it not "taken". It is provided. If you want your "own business-logic", you implement it and you use it. There is nothing stopping you from doing that, right? Unless I missed, mis-understood something.
Myabe you could reflect that in the initial examples by not providing `operator bool` and `operator!`? BTW, you probably do not need `operator!` anyway. The negation works out of the box when you only define explicit conversion to bool. Regards, &rzej;
Andrzej, I'll just answer top-post as we are not exactly arguing any particular implementation/documentation points. We just happen to see things from somewhat different angles. I understand that you've had bad experiences with shared_ptrs and such and nulls. Fair enough. So, if you do decide to use impl_ptr library, you'll avoid impl_ptr::shared policy and will use others. From what I understand those "unique", "copied", "onstack" policies should fit your needs and address your null-related concerns. Your concerns of objects being uninitialized (null) are unfounded. We can discuss it later in depth when the time comes. As for the docs, then they certainly can and should be improved... is now the time to spend time improving them?.. I personally suspect not because impl_ptr library is in the "endorsement request" stage... it is not even a review stage but some strange pre-review stage... and so far I cannot see it making any further. So, we'll address issues raised when it is appropriate (given that those issues so far have not been serious architectural/design issues but documentation and potential misunderstanding). Best, V. On 2017-08-22 18:09, Andrzej Krzemienski via Boost wrote:
2017-08-22 2:34 GMT+02:00 Vladimir Batov via Boost <boost@lists.boost.org>:
On 08/21/2017 10:38 PM, Andrzej Krzemienski via Boost wrote:
2017-08-15 2:51 GMT+02:00 Vladimir Batov via Boost <boost@lists.boost.org
After much procrastination I would like to request a formal review for the pimpl library...
I find it disturbing that the first example describes using shared_ptr-s and defensive null checks. Boost has always been promoting the STL-like value-semantic style of programming, and this usage of shared_ptr's looks counter to the trend. When I am defining type Book -- unless I have to hide implementation, I will just put the members directly in order to achieve value semantics, in particular, I want to guarantee the following property: Book b1 {"title", "author"}; Book b2 = b1; Book b3 = b1; modify(b1); assert (b2 == b3); assert (b2 != b1); The shared_ptr does no seem to guarantee the last assertion. Also, there are these additional members testing if pimpl is null, which appear suspicious to me. How can it be null? Do I have to implement the default constructor now? Does this also mean than in my types that I want to pimpl-ize I cannot use operator bool for my own business-logic-related purpose, because it is already taken by the implementation of boost::pimpl? There may be good use cases for flyweight-like implementation, but it should not necessarily be the first example we see in the docs.
Andrzej,
To be honest I am somewhat surprised... in part by the fact that you find the first example no less than "disturbing". I do hear your view and I find it is a perfectly valid view. Still, I am sure you've been around the bush for long enough to know that there are as many views/opinions/preferences as there are people. Then different projects have different requirements. So, there should not be anything "disturbing" if you see someone doing something his way rather than yours to satisfy the requirements of their project rather than yours. Don't you agree?
Then you go on to explain that the shared-properties pimpl does not suit your requirements and to describe your potential confusion (and even frustration) with such pimpls... Where all that is coming from? No one is forcing you to use the shared-properties pimpl, right? Exclusive-ownership pimpls are equal-rights' citizens in the implementation. They are equally presented in the documentation. What is there to be "disturbed" by? Indeed, in the documentation I do mention/describe the shared-properties pimpl first... because it is easier to introduce the Pimpl concept using std::shared_ptr. Different people have different skill/knowledge levels, different expectations, different priorities, different reading styles. There is no way to satisfy them all. Please do not be "disturbed" if you do not see things done exactly you'd do that.
Part of my complaint is indeed coming from my personal judgement of different implementation strategies. The other part is me giving you a feedback from the "user experience" of a potential user of your library. I am an impatient programmer who is facing alternative: implement pimpl manually or use a third-party library, there might be like 100 libraries for this purpose with different quality, and I have very little time to asses if it is better to use one of them, or to write my own. Having encountered a library I need to be able to answer a couple of questions quickly, like, is this library worth investing time in even reading the full documentation. So, one of the first questions I would like being answered is. Is the author of this library aware of the present programming styles, like value semantics, zombie-object avoidance, or is (s)he confined to these older OO-like techniques, where everything is pointers and garbage-collected (or shared_ptr-collected) memory with null pointers causing bugs?
I cannot figure this out from the docs of Boost.Pimpl quickly. Ultimately, I can figue it out from studying the entire docs, or the implementations, or private conversation with you. But: I cannot figure it out *quickly* from the *initial pages* of the docs. First page: no information -- I know what pimpl is already. Second page: "the basics" -- I do not want the basics. I want answers, preferably in form of a short example. There is a word "unique-ownership" highlighted, so this is something, but still some questions unanswered: will I have null pointers/null states all around?
Third page: I get an example of something I would not like to get. I give you that it your order of presenting policies is no worse than my expected order of policies, but I can see these two functions in the interface for testing for the null state. Note that separating interface from implementation has nothing to do with offering an additional null state. The two things are orthogonal; it is just that it is easy to provide a null state when you are implementing a handle anyway -- but they are different things, and they distract attention. It is not clear from the first glimpse at the example whether I am forced to have the null state or whether I can opt-in to have it.
By that time I will have exhausted the time I was willing to devote to studying the library usefulness, and would move to the next library or to implementing my own pimpl.
Note that my remarks are to the structure of the initial pages of the docs: not to your design choices.
Also, there are these additional members testing if pimpl is null, which appear suspicious to me. How can it be null?
std::optional has relational operators and can be "null", invalid. std::shared_ptr() has relational operators and can be "null", invalid. unique_ptr has relational operators and can be "null", invalid. The class I proposed -- impl_ptr -- has relational operators and can be "null", invalid. What is so suspicious about it? The name itself -- impl_ptr -- highlights the fact that it is a smart pointer, a handle/body construct. So, the handle can be associated with a body... or the handle might not have a body, i.e. "invalid", "null".
Of optional, I expect a null state. This is what optional is for and this is reflected in its type. Of shared_ptr I do not necessarily expect a null state, and I treat it as necessary evil, and long for types with shared semantics but without null state which is causing many bugs in my programs I work with. Of "pimpl" library I expect separation of interface and implementation: not a null state. If you provide it, I would expect a clear indication that it is an opt-in feature.
Do I have to implement the default constructor now?
No, you do not. Your frustration seems palpable. I fail to understand how possibly I could cause that.
It is not you causing it. Maybe I am just a nerd. I have bad experience with having to spend lot of time in my company fixing bugs after people who overused OO and shared_ptrs. I see now the docs of Boost.Pimpl unnecessarily promoting these. Doing flyweighs requires taking care of oher things like using shared_ptr<const T> rather than shared_ptr<mutable T>, which the docs do not cover.
Does this also mean than in my types that I want to pimpl-ize I cannot use operator bool for my own business-logic-related purpose, because it is already taken by the implementation of boost::pimpl?
No, it does not mean anything like you describe... and it not "taken". It is provided. If you want your "own business-logic", you implement it and you use it. There is nothing stopping you from doing that, right? Unless I missed, mis-understood something.
Myabe you could reflect that in the initial examples by not providing `operator bool` and `operator!`? BTW, you probably do not need `operator!` anyway. The negation works out of the box when you only define explicit conversion to bool.
While I like the idea in general (bikeshedding aside), it should not be possible to end up with nulls when treating something as a value type. All of that should be hidden away with some sort of enable if machinery. Also your support for allocators doesn't work properly because if you use something like an allocator that returns offset pointers you assume that all allocators return normal pointers. On Tue, Aug 22, 2017 at 5:49 AM, Vladimir Batov via Boost <boost@lists.boost.org> wrote:
Andrzej,
I'll just answer top-post as we are not exactly arguing any particular implementation/documentation points. We just happen to see things from somewhat different angles. I understand that you've had bad experiences with shared_ptrs and such and nulls. Fair enough. So, if you do decide to use impl_ptr library, you'll avoid impl_ptr::shared policy and will use others. From what I understand those "unique", "copied", "onstack" policies should fit your needs and address your null-related concerns. Your concerns of objects being uninitialized (null) are unfounded. We can discuss it later in depth when the time comes.
As for the docs, then they certainly can and should be improved... is now the time to spend time improving them?.. I personally suspect not because impl_ptr library is in the "endorsement request" stage... it is not even a review stage but some strange pre-review stage... and so far I cannot see it making any further. So, we'll address issues raised when it is appropriate (given that those issues so far have not been serious architectural/design issues but documentation and potential misunderstanding).
Best, V.
On 2017-08-22 18:09, Andrzej Krzemienski via Boost wrote:
2017-08-22 2:34 GMT+02:00 Vladimir Batov via Boost <boost@lists.boost.org>:
On 08/21/2017 10:38 PM, Andrzej Krzemienski via Boost wrote:
2017-08-15 2:51 GMT+02:00 Vladimir Batov via Boost <boost@lists.boost.org
After much procrastination I would like to request a formal review for the pimpl library...
I find it disturbing that the first example describes using shared_ptr-s and defensive null checks. Boost has always been promoting the STL-like value-semantic style of programming, and this usage of shared_ptr's looks counter to the trend. When I am defining type Book -- unless I have to hide implementation, I will just put the members directly in order to achieve value semantics, in particular, I want to guarantee the following property: Book b1 {"title", "author"}; Book b2 = b1; Book b3 = b1; modify(b1); assert (b2 == b3); assert (b2 != b1); The shared_ptr does no seem to guarantee the last assertion. Also, there are these additional members testing if pimpl is null, which appear suspicious to me. How can it be null? Do I have to implement the default constructor now? Does this also mean than in my types that I want to pimpl-ize I cannot use operator bool for my own business-logic-related purpose, because it is already taken by the implementation of boost::pimpl? There may be good use cases for flyweight-like implementation, but it should not necessarily be the first example we see in the docs.
Andrzej,
To be honest I am somewhat surprised... in part by the fact that you find the first example no less than "disturbing". I do hear your view and I find it is a perfectly valid view. Still, I am sure you've been around the bush for long enough to know that there are as many views/opinions/preferences as there are people. Then different projects have different requirements. So, there should not be anything "disturbing" if you see someone doing something his way rather than yours to satisfy the requirements of their project rather than yours. Don't you agree?
Then you go on to explain that the shared-properties pimpl does not suit your requirements and to describe your potential confusion (and even frustration) with such pimpls... Where all that is coming from? No one is forcing you to use the shared-properties pimpl, right? Exclusive-ownership pimpls are equal-rights' citizens in the implementation. They are equally presented in the documentation. What is there to be "disturbed" by? Indeed, in the documentation I do mention/describe the shared-properties pimpl first... because it is easier to introduce the Pimpl concept using std::shared_ptr. Different people have different skill/knowledge levels, different expectations, different priorities, different reading styles. There is no way to satisfy them all. Please do not be "disturbed" if you do not see things done exactly you'd do that.
Part of my complaint is indeed coming from my personal judgement of different implementation strategies. The other part is me giving you a feedback from the "user experience" of a potential user of your library. I am an impatient programmer who is facing alternative: implement pimpl manually or use a third-party library, there might be like 100 libraries for this purpose with different quality, and I have very little time to asses if it is better to use one of them, or to write my own. Having encountered a library I need to be able to answer a couple of questions quickly, like, is this library worth investing time in even reading the full documentation. So, one of the first questions I would like being answered is. Is the author of this library aware of the present programming styles, like value semantics, zombie-object avoidance, or is (s)he confined to these older OO-like techniques, where everything is pointers and garbage-collected (or shared_ptr-collected) memory with null pointers causing bugs?
I cannot figure this out from the docs of Boost.Pimpl quickly. Ultimately, I can figue it out from studying the entire docs, or the implementations, or private conversation with you. But: I cannot figure it out *quickly* from the *initial pages* of the docs. First page: no information -- I know what pimpl is already. Second page: "the basics" -- I do not want the basics. I want answers, preferably in form of a short example. There is a word "unique-ownership" highlighted, so this is something, but still some questions unanswered: will I have null pointers/null states all around?
Third page: I get an example of something I would not like to get. I give you that it your order of presenting policies is no worse than my expected order of policies, but I can see these two functions in the interface for testing for the null state. Note that separating interface from implementation has nothing to do with offering an additional null state. The two things are orthogonal; it is just that it is easy to provide a null state when you are implementing a handle anyway -- but they are different things, and they distract attention. It is not clear from the first glimpse at the example whether I am forced to have the null state or whether I can opt-in to have it.
By that time I will have exhausted the time I was willing to devote to studying the library usefulness, and would move to the next library or to implementing my own pimpl.
Note that my remarks are to the structure of the initial pages of the docs: not to your design choices.
Also, there are these additional members testing if pimpl is null, which appear suspicious to me. How can it be null?
std::optional has relational operators and can be "null", invalid. std::shared_ptr() has relational operators and can be "null", invalid. unique_ptr has relational operators and can be "null", invalid. The class I proposed -- impl_ptr -- has relational operators and can be "null", invalid. What is so suspicious about it? The name itself -- impl_ptr -- highlights the fact that it is a smart pointer, a handle/body construct. So, the handle can be associated with a body... or the handle might not have a body, i.e. "invalid", "null".
Of optional, I expect a null state. This is what optional is for and this is reflected in its type. Of shared_ptr I do not necessarily expect a null state, and I treat it as necessary evil, and long for types with shared semantics but without null state which is causing many bugs in my programs I work with. Of "pimpl" library I expect separation of interface and implementation: not a null state. If you provide it, I would expect a clear indication that it is an opt-in feature.
Do I have to implement the default constructor now?
No, you do not. Your frustration seems palpable. I fail to understand how possibly I could cause that.
It is not you causing it. Maybe I am just a nerd. I have bad experience with having to spend lot of time in my company fixing bugs after people who overused OO and shared_ptrs. I see now the docs of Boost.Pimpl unnecessarily promoting these. Doing flyweighs requires taking care of oher things like using shared_ptr<const T> rather than shared_ptr<mutable T>, which the docs do not cover.
Does this also mean than in my types that I want to pimpl-ize I cannot use operator bool for my own business-logic-related purpose, because it is already taken by the implementation of boost::pimpl?
No, it does not mean anything like you describe... and it not "taken". It is provided. If you want your "own business-logic", you implement it and you use it. There is nothing stopping you from doing that, right? Unless I missed, mis-understood something.
Myabe you could reflect that in the initial examples by not providing `operator bool` and `operator!`? BTW, you probably do not need `operator!` anyway. The negation works out of the box when you only define explicit conversion to bool.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On 08/23/2017 01:06 AM, Gary Furnish via Boost wrote:
While I like the idea in general (bikeshedding aside), it should not be possible to end up with nulls when treating something as a value type.
Is it a general observation or you have a particular use-case in mind that involves the discussed impl_ptr? It that is the former, then IMO it is not an issue as impl_ptr is a base class and the user/interface/proxy class ultimately decides if it is "possible to end up with nulls" or not. If that is the latter, then I'd certainly like to look at it and to evaluate and to fix if necessary.
All of that should be hidden away with some sort of enable if machinery. Also your support for allocators doesn't work properly because if you use something like an allocator that returns offset pointers you assume that all allocators return normal pointers.
Well, "your support for allocators doesn't work properly" seems like a sweeping statement. They "seem" to work for me but you must have a particular use-case in mind where they do not. Do you mind elaborating, sharing it? I am somewhat puzzled by the meaning of "offset pointers". I presume it is a pointer offset from the beginning of actually allocated block, right? If so, does std::allocator support that?.. impl_ptr uses the facilities provided by the allocator concept. Namely, if your particular allocator returns an "offset pointer" from allocate(), then that same pointer is given back to your allocator to deal with inside deallocate(). So, where does "your support for allocators doesn't work properly" come in?
On Tue, Aug 22, 2017 at 3:18 PM, Vladimir Batov via Boost <boost@lists.boost.org> wrote:
On 08/23/2017 01:06 AM, Gary Furnish via Boost wrote:
While I like the idea in general (bikeshedding aside), it should not be possible to end up with nulls when treating something as a value type.
Is it a general observation or you have a particular use-case in mind that involves the discussed impl_ptr? It that is the former, then IMO it is not an issue as impl_ptr is a base class and the user/interface/proxy class ultimately decides if it is "possible to end up with nulls" or not. If that is the latter, then I'd certainly like to look at it and to evaluate and to fix if necessary.
On the nulls: just looking it may be policy dependent if you can end up with nulls in the sense of runtime, but the machinery for nulls will still show up in auto completion,name resolution, etc unless it is enable-ifed or specialized out. It is possible I mistook how this works though. I don't see a problem with supporting shared situations with policies, but it should be 100% disabled. No static other_type null() { return boost_impl_ptr_detail<other_type>::null(); } static user_type null() { return boost_impl_ptr_detail< user_type>::null(); }
All of that should be hidden away with some sort of enable if machinery. Also your support for allocators doesn't work properly because if you use something like an allocator that returns offset pointers you assume that all allocators return normal pointers.
Well, "your support for allocators doesn't work properly" seems like a sweeping statement. They "seem" to work for me but you must have a particular use-case in mind where they do not. Do you mind elaborating, sharing it? I am somewhat puzzled by the meaning of "offset pointers". I presume it is a pointer offset from the beginning of actually allocated block, right? If so, does std::allocator support that?.. impl_ptr uses the facilities provided by the allocator concept. Namely, if your particular allocator returns an "offset pointer" from allocate(), then that same pointer is given back to your allocator to deal with inside deallocate(). So, where does "your support for allocators doesn't work properly" come in?
Offset pointers are pointers that you use with an allocator where the pointer is actually a size_t that is added to the location of the pointer. This is used in mmap situations where you do not know the absolute address, only relative to the current pointer offset. See http://www.boost.org/doc/libs/1_65_0/doc/html/interprocess/offset_ptr.html So you just define an allocator where Allocator::pointer = boost::offset_ptr<T> and vector etc automatically work in shared memory. The basic punchline is instead of hardcoding the allocator return value as T* you want to capture a std::allocator_traits<...>::pointer . You can then use std::address_of(*raw memory) to get the raw memory address. (Note address of because operator& could be overloaded). (This is how to do it in c++17, earlier versions require a bit more boiler plate). It is not true allocator support if you don't support the rarer cases.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On Tue, Aug 22, 2017 at 5:41 PM, Gary Furnish wrote:
You can then use std::address_of(*raw memory) to get the raw memory address.
addressof(*p) to construct an object (e.g. via placement new or via an allocator) is undefined if p points to storage without an object constructed in it. It is the reason libstdc++, libc++, Boost use to_raw_pointer(), to_address() functions). See P0653r1.[1] boost::to_address(p) instead should be fine. [1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0653r1.html
On 08/23/2017 07:41 AM, Gary Furnish via Boost wrote:
On Tue, Aug 22, 2017 at 3:18 PM, Vladimir Batov via Boost <boost@lists.boost.org> wrote:
While I like the idea in general (bikeshedding aside), it should not be possible to end up with nulls when treating something as a value type. Is it a general observation or you have a particular use-case in mind that involves the discussed impl_ptr? It that is the former, then IMO it is not an issue as impl_ptr is a base class and the user/interface/proxy class
On 08/23/2017 01:06 AM, Gary Furnish via Boost wrote: ultimately decides if it is "possible to end up with nulls" or not. If that is the latter, then I'd certainly like to look at it and to evaluate and to fix if necessary. On the nulls: just looking it may be policy dependent if you can end up with nulls in the sense of runtime, but the machinery for nulls will still show up in auto completion,name resolution, etc unless it is enable-ifed or specialized out. It is possible I mistook how this works though. I don't see a problem with supporting shared situations with policies, but it should be 100% disabled. No
static other_type null() { return boost_impl_ptr_detail<other_type>::null(); } static user_type null() { return boost_impl_ptr_detail< user_type>::null(); }
Sure. If taking these convenience interfaces out makes people happy, I have nothing against it. No drama. I guess, I'll go take it out right away. :-)
All of that should be hidden away with some sort of enable if machinery. Also your support for allocators doesn't work properly because if you use something like an allocator that returns offset pointers you assume that all allocators return normal pointers. Well, "your support for allocators doesn't work properly" seems like a sweeping statement. They "seem" to work for me but you must have a particular use-case in mind where they do not. Do you mind elaborating, sharing it? I am somewhat puzzled by the meaning of "offset pointers". I presume it is a pointer offset from the beginning of actually allocated block, right? If so, does std::allocator support that?.. impl_ptr uses the facilities provided by the allocator concept. Namely, if your particular allocator returns an "offset pointer" from allocate(), then that same pointer is given back to your allocator to deal with inside deallocate(). So, where does "your support for allocators doesn't work properly" come in?
Offset pointers are pointers that you use with an allocator where the pointer is actually a size_t that is added to the location of the pointer. This is used in mmap situations where you do not know the absolute address, only relative to the current pointer offset. See http://www.boost.org/doc/libs/1_65_0/doc/html/interprocess/offset_ptr.html So you just define an allocator where Allocator::pointer = boost::offset_ptr<T> and vector etc automatically work in shared memory. The basic punchline is instead of hardcoding the allocator return value as T* you want to capture a std::allocator_traits<...>::pointer . You can then use std::address_of(*raw memory) to get the raw memory address. (Note address of because operator& could be overloaded). (This is how to do it in c++17, earlier versions require a bit more boiler plate). It is not true allocator support if you don't support the rarer cases.
Uh, right. Thank you for the link and explanations. Appreciated. And you are certainly correct, I should be using std::allocator_traits<...>::pointer. That said, I consider it an oversight, a nit to be addressed in due course... Agree?
On Tue, Aug 22, 2017 at 4:02 PM, Vladimir Batov via Boost <boost@lists.boost.org> wrote:
On 08/23/2017 07:41 AM, Gary Furnish via Boost wrote:
On Tue, Aug 22, 2017 at 3:18 PM, Vladimir Batov via Boost <boost@lists.boost.org> wrote:
On 08/23/2017 01:06 AM, Gary Furnish via Boost wrote:
While I like the idea in general (bikeshedding aside), it should not be possible to end up with nulls when treating something as a value type.
Is it a general observation or you have a particular use-case in mind that involves the discussed impl_ptr? It that is the former, then IMO it is not an issue as impl_ptr is a base class and the user/interface/proxy class ultimately decides if it is "possible to end up with nulls" or not. If that is the latter, then I'd certainly like to look at it and to evaluate and to fix if necessary.
On the nulls: just looking it may be policy dependent if you can end up with nulls in the sense of runtime, but the machinery for nulls will still show up in auto completion,name resolution, etc unless it is enable-ifed or specialized out. It is possible I mistook how this works though. I don't see a problem with supporting shared situations with policies, but it should be 100% disabled. No
static other_type null() { return boost_impl_ptr_detail<other_type>::null(); } static user_type null() { return boost_impl_ptr_detail< user_type>::null(); }
Sure. If taking these convenience interfaces out makes people happy, I have nothing against it. No drama. I guess, I'll go take it out right away. :-)
All of that should be hidden away with some sort of enable if machinery. Also your support for allocators doesn't work properly because if you use something like an allocator that returns offset pointers you assume that all allocators return normal pointers.
Well, "your support for allocators doesn't work properly" seems like a sweeping statement. They "seem" to work for me but you must have a particular use-case in mind where they do not. Do you mind elaborating, sharing it? I am somewhat puzzled by the meaning of "offset pointers". I presume it is a pointer offset from the beginning of actually allocated block, right? If so, does std::allocator support that?.. impl_ptr uses the facilities provided by the allocator concept. Namely, if your particular allocator returns an "offset pointer" from allocate(), then that same pointer is given back to your allocator to deal with inside deallocate(). So, where does "your support for allocators doesn't work properly" come in?
Offset pointers are pointers that you use with an allocator where the pointer is actually a size_t that is added to the location of the pointer. This is used in mmap situations where you do not know the absolute address, only relative to the current pointer offset. See http://www.boost.org/doc/libs/1_65_0/doc/html/interprocess/offset_ptr.html So you just define an allocator where Allocator::pointer = boost::offset_ptr<T> and vector etc automatically work in shared memory. The basic punchline is instead of hardcoding the allocator return value as T* you want to capture a std::allocator_traits<...>::pointer . You can then use std::address_of(*raw memory) to get the raw memory address. (Note address of because operator& could be overloaded). (This is how to do it in c++17, earlier versions require a bit more boiler plate). It is not true allocator support if you don't support the rarer cases.
Uh, right. Thank you for the link and explanations. Appreciated. And you are certainly correct, I should be using std::allocator_traits<...>::pointer. That said, I consider it an oversight, a nit to be addressed in due course... Agree?
Yeah, I like the general idea and I like not having 5000 implementations of similar things floating around. I need to study it a bit more, those were just my initial 5 min check suggestions.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On 08/23/2017 08:18 AM, Gary Furnish via Boost wrote:
Yeah, I like the general idea and I like not having 5000 implementations of similar things floating around. I need to study it a bit more, those were just my initial 5 min check suggestions.
You nailed it. That is exactly why I feel it is useful. When I look at it in isolation, as an individual developer, I think "meh, big deal". When I have to review somebody else's code day in and day out, then being able to instantly see/recognize an applied/used pattern is hugely important. That makes the reviewer's life so much easier, the review much quicker and more robust. In safety-related industries (air, rail, military) those are an essential part of dev. process. Having to review 5 submissions, analyze what the hell's going on there, what patterns they implement, if they missed/misused copy/move constructors/assignments is damn mind-numbing. Same goes for code maintenance. impl_ptr helps great deal IMO.
participants (9)
-
Andrzej Krzemienski -
degski -
Gary Furnish -
Gavin Lambert -
Glen Fernandes -
Hans Dembinski -
Seth -
Vicente J. Botet Escriba -
Vladimir Batov