Re: [boost] Review Request: impl_ptr (pimpl)

On 2017-08-19 23:55, Mario Lang wrote:

Vladimir Batov via Boost 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.