Review Request: impl_ptr (pimpl)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
44 messages Options
123
Reply | Threaded
Open this post in threaded view
|

Review Request: impl_ptr (pimpl)

Boost - Dev mailing list
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
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: impl_ptr (pimpl)

Boost - Dev mailing list
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


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: impl_ptr (pimpl)

Boost - Dev mailing list
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.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: impl_ptr (pimpl)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
2017-08-15 2:51 GMT+02:00 Vladimir Batov via Boost <[hidden email]>:

> 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;

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: impl_ptr (pimpl)

Boost - Dev mailing list
On 2017-08-16 18:14, Andrzej Krzemienski via Boost wrote:
> 2017-08-15 2:51 GMT+02:00 Vladimir Batov via Boost
> <[hidden email]>:
> ...
> 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.








_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: impl_ptr (pimpl)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Hi Vladimir,

> On 15. Aug 2017, at 02:51, Vladimir Batov via Boost <[hidden email]> 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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: impl_ptr (pimpl)

Boost - Dev mailing list
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 <[hidden email]> 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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: impl_ptr (pimpl)

Boost - Dev mailing list
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.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

[impl_ptr] Endorsement Request

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: impl_ptr (pimpl)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list

> On 15. Aug 2017, at 02:51, Vladimir Batov via Boost <[hidden email]> 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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: impl_ptr (pimpl)

Boost - Dev mailing list
On 2017-08-18 19:50, Hans Dembinski wrote:
>> On 15. Aug 2017, at 02:51, Vladimir Batov via Boost
>> <[hidden email]> 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.






_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: impl_ptr (pimpl)

Boost - Dev mailing list
On 18 August 2017 at 13:47, Vladimir Batov via Boost <[hidden email]>
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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: impl_ptr (pimpl)

Boost - Dev mailing list
On 2017-08-18 23:05, degski via Boost wrote:

> On 18 August 2017 at 13:47, Vladimir Batov via Boost
> <[hidden email]>
> 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.



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: impl_ptr (pimpl)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 2017-08-19 23:55, Mario Lang wrote:
> Vladimir Batov via Boost <[hidden email]> writes:
> ...
> I got interested enough by this posting so that I ended up doing my own
> mini-review just right now :-).  And I just wanted to let you know (and
> encourage you) that I like this one alot.  Keeping the public interface
> free of boilerplate is quite nice!

Mario,

Thank you for your input. Much appreciated... although somewhat early.
:-) It was pointed out to me that Boost review procedures changed and a
proposal has to be endorsed before it can move to the review stage. So
far I do not see it happening... maybe because the list is quiet
(overall), maybe because iml_ptr is not good enough to attract that
support/endorsement, maybe giving "endorsement" puts too much pressure
and people shy away from it. We'll see.

> I think for a full boost review, you will probably need to work on:
> * The documentation
>   While I gave it a read, I ended up skipping to your test case because
>   I wanted to see working example code.
>   While the current docs have quite some examples, they spend a lot
> more
>   screen space with showing what impl_ptr tries to replace, without
>   showing a good motivating positive example of how it actually looks
>   when employed.  It also feels like it is spending some time with
>   marketing speak, while not really showing what it is trying to sell:

Yes, I hear you... and I (at least partially) agree with you. First, no
code or documentation will ever be to everyone's liking... unless
everyone writes their own. Secondly, the documentation reflects many
discussions we've had on this list about pimpl. For ex., I had to
explicitly add what you call "marketing speak" to plainly explain why
IMO pimpl is important. I had to address numerous "why bother"
dismissing comments. What you call "marketing speak" I call an
architectural view of a project. IMO it's quite different from
developer's view that you seem to favor. All views are important... and
all of them IMO need to be mentioned to cater for and to convince
different readers. Surely, you (as a developer) is free to skip the
"marketing speak". Somebody else (an architect and code-reviewer) might
decide to skip other bits. No drama. Does it make sense?

>   The separation between the public interface in a hpp and the template
>   specialization in a cpp.  I think the motivating example should
>   clearly show what part of the code goes where.

Isn't what I did in third and fourth chapters? That said, I'll never be
able to write the way you (or somebody else) like it. So, if you feel
really strongly about it, please feel free to re-write it as you feel
proper and submit. I am more than happy to consider and take others
help. Unfortunately, that'll reverse our positions as I (and potential
reviewers) might be saying "nuh, not enough "marketing speak" and such.
No pressure... but that's an option to shape it up to your liking.

> * Namespace:
>   While the alias is good, I think the whole code should be moved to
>   boost::, and the detail to boost::detail::impl_ptr::.
>   I am sort of guessing this will come up in the review.
>   I might be interested to help with that and submit the renaming PR if
>   you are open to that.

1) I do agree that the "whole code should be moved to boost::"... when
it is part of Boost. Currently it is not. Currently I am on the earliest
stage of gathering interest and waiting for "endorsement" and the review
manager.

2) As I think I mentioned before impl_ptr is not part of any namespace
by design... because the way proxy and implementation are connected, the
specialization of impl_ptr<>::implementation. As I also mentioned it is
IMO only a "problem" for "purists" but not for "practitioners" as from
outside it is all kosher and accessed via boost::impl_ptr.

You might be interested in searching for "Pimpl Again?" thread on this
list for prev. discussions.

Still, it you know a way around it and willing to submit your
suggestions, I am certainly open for and most welcome it. But just to
state the obvious -- as you do not like every line of code/doc that I
write -- that goes both ways.








_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: impl_ptr (pimpl)

Boost - Dev mailing list
2017-08-20 0:46 GMT+02:00 Vladimir Batov via Boost <[hidden email]>:

>
>
> 2) As I think I mentioned before impl_ptr is not part of any namespace by
> design... because the way proxy and implementation are connected, the
> specialization of impl_ptr<>::implementation. As I also mentioned it is IMO
> only a "problem" for "purists" but not for "practitioners" as from outside
> it is all kosher and accessed via boost::impl_ptr.
>

Could you summarize why using the global namespace is necessary? Is it only
to be able to write

  template<> class _boost_impl_ptr_

instead of

  namespace boost { template<> class impl_ptr }

or something more?

Maybe it is described in the docs somewhere, but I couldn't find it.

Regards,
&rzej;

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: impl_ptr (pimpl)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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_interface.html 
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_exclusive_ownership_p.html),
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_impl_ptr_null.html,
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.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: impl_ptr (pimpl)

Boost - Dev mailing list
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_interface.html
> 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_impl_ptr_null.html,
> 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.



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: impl_ptr (pimpl)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list

>> 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_containers.html

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-allocator

and also the explanation here, lines 23 to 35:
https://src.chromium.org/viewvc/chrome/trunk/src/base/containers/stack_container.h?view=markup

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.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: impl_ptr (pimpl)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 2017-08-21 16:29, Andrzej Krzemienski via Boost wrote:

> 2017-08-20 0:46 GMT+02:00 Vladimir Batov via Boost
> <[hidden email]>:
>>
>> 2) As I think I mentioned before impl_ptr is not part of any namespace
>> by
>> design... because the way proxy and implementation are connected, the
>> specialization of impl_ptr<>::implementation. As I also mentioned it
>> is IMO
>> only a "problem" for "purists" but not for "practitioners" as from
>> outside
>> it is all kosher and accessed via boost::impl_ptr.
>
> Could you summarize why using the global namespace is necessary?

template<> struct impl_ptr<Foo>::implementation is a template
specialization. So, it has to be defined in the original namespace. So,
if it is

namespace boost
{
     template<typename> struct impl_ptr;
}

then the developer will have to declare his specialization of
"implementation" inside boost:

namespace boost
{
     template<> struct impl_ptr<Foo>::implementation { ... };
}

That's ugly and wrong. So, I did

template<typename> struct _internal_boost_impl_ptr;

namespace boost
{
     template<typename T> using impl_ptr = ::_internal_boost_impl_ptr<T>;
}

Now for all purposes impl_ptr appears as "boost::impl_ptr". Then, the
developer defines his "implementation" specialization like

template<> struct boost::impl_ptr<Foo>::implementation { ... };

which looks sensible IMO.



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: impl_ptr (pimpl)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
123