Re: [boost] Review Request: impl_ptr (pimpl)
On 2017-08-19 23:55, Mario Lang wrote:
Vladimir Batov via Boost <boost@lists.boost.org> 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.
2017-08-20 0:46 GMT+02:00 Vladimir Batov via Boost <boost@lists.boost.org>:
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;
On 2017-08-21 16:29, Andrzej Krzemienski via Boost wrote:
2017-08-20 0:46 GMT+02:00 Vladimir Batov via Boost <boost@lists.boost.org>:
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.
On 21.08.2017 13:48, Vladimir Batov via Boost wrote:
On 2017-08-21 16:29, Andrzej Krzemienski via Boost wrote:
2017-08-20 0:46 GMT+02:00 Vladimir Batov via Boost <boost@lists.boost.org>:
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. <snip>
FWIW, as a Boost user I don't find that ugly and wrong. Plus we already do things like namespace boost { void assertion_failed(char const * expr, char const * function, char const * file, long line) { // ... } } so for a long time now, we have a precedent too. Regards, Gevorg
On 21-08-17 11:48, Vladimir Batov via Boost wrote:
That's ugly and wrong.
/I'm just gonna say it's neither ugly nor wrong./ On 22-08-17 09:50, Gevorg Voskanyan via Boost wrote:
so for a long time now, we have a precedent too.
One of a great many. Same for specializations of std::hash, boost::hash, boost::fusion::extension::*, boost::spirit::traits::*, etc. That said, I'm not convinced there isn't a "better" way: why not implement another layer of indirection. Make it a trait: boost::impl_ptr::traits::implementation_for<T, Enable>::type could be made to refer to the user-defined implementation type. That way the "pain" is only in 1 line where the trait is specialized.
On 08/23/2017 07:07 AM, Seth via Boost wrote:
On 21-08-17 11:48, Vladimir Batov via Boost wrote:
That's ugly and wrong. /I'm just gonna say it's neither ugly nor wrong./
OK, OK. :-) We certainly won't be arguing that. :-) I just thought that the words themselves clearly indicated that it was in my view, by my taste. I do not feel comfortable seeing the code that looks like the user introduces something, fiddles in the boost (or std) namespace. I have a feeling I might be not the only one feeling that way. :-)
On 22-08-17 09:50, Gevorg Voskanyan via Boost wrote:
so for a long time now, we have a precedent too.
Gevorg was certainly correct pointing out this boost::assertion_failed() precedent... Something I have in my code. :-)
One of a great many.
Same for specializations of std::hash, boost::hash, boost::fusion::extension::*, boost::spirit::traits::*, etc.
That said, I'm not convinced there isn't a "better" way: why not implement another layer of indirection. Make it a trait:
boost::impl_ptr::traits::implementation_for<T, Enable>::type
could be made to refer to the user-defined implementation type. That way the "pain" is only in 1 line where the trait is specialized.
Before we embark on exploring other possibilities, is there something in particular that you object to in the proposed template<> struct boost::impl_ptr<Foo>::implementation declaration?
On 2017-08-24 16:54, Seth via Boost wrote:
On 22-08-17 23:37, Vladimir Batov via Boost wrote:
template<> struct boost::impl_ptr<Foo>::implementation
not at all. I believe it is you who named that "ugly and wrong". So there you go.
Thank you for clarifications. Appreciated. All good then. :-) I was referring to something similar but different... but obviously muddled it all up. Moving on. :-)
How is the review process moving on? I have a clear need for this in my code right now. Would love to see it get protection from obscurity by being accepted into boost sometime soon. On 24 August 2017 at 13:34, Vladimir Batov via Boost <boost@lists.boost.org> wrote:
On 2017-08-24 16:54, Seth via Boost wrote:
On 22-08-17 23:37, Vladimir Batov via Boost wrote:
template<> struct boost::impl_ptr<Foo>::implementation
not at all. I believe it is you who named that "ugly and wrong". So there you go.
Thank you for clarifications. Appreciated. All good then. :-) I was referring to something similar but different... but obviously muddled it all up. Moving on. :-)
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman /listinfo.cgi/boost
On 2017-09-09 01:34, Richard Hodges via Boost wrote:
How is the review process moving on? I have a clear need for this in my code right now.
Would love to see it get protection from obscurity by being accepted into boost sometime soon.
Thank you for your interest and encouragement. Much appreciated. RE: review As for the actual review, then (unfortunately) there is no one. From my understanding the Boost review process has changed and now a submission is only scheduled for a review IFF it gets a review manager. It is not a position people queue for. :-) So, no one has come forward for impl_ptr to be a review manager... as I can see for other submissions also... Before such a manager-less submission would be put in the queue and stay on the radar... Now such a submission generates initial interest on the list, then drifts out of the scope and is left behind/forgotten. It's unfortunate. RE: obscurity You might consider going to https://github.com/yet-another-user/pimpl and adding a star to the project. It raises its visibility in a GitHub search with everything following. RE: accepted into boost Initially I personally had my doubts if it was not too simple, obvious and basic. Now Giel van Schijndel joined in and made immense contributions/improvements to all policies. Namely, 'unique' and 'copied' policies are std::unique_ptr-based and pimpl-objects are of the 'void*' size... no memory overhead!.. Hugely useful IMO. Then, for high performance two in-place (no dyn. mem. allocation) policies are really well-done with one such policy not having any mem. overhead at all. So, IMO the submission has certainly something to offer functionality-wise and deployment-wise beyond manual pimpl-idiom implementation and would be a useful addition to the existing set of smart pointers...
Hi Vladimir, great to see that good has come of the review process. If you'll allow my first commit to add a CMakeLists.txt file I'll happily just start using it and contributing where possible. On 8 September 2017 at 22:53, Vladimir Batov via Boost < boost@lists.boost.org> wrote:
On 2017-09-09 01:34, Richard Hodges via Boost wrote:
How is the review process moving on? I have a clear need for this in my code right now.
Would love to see it get protection from obscurity by being accepted into boost sometime soon.
Thank you for your interest and encouragement. Much appreciated.
RE: review
As for the actual review, then (unfortunately) there is no one. From my understanding the Boost review process has changed and now a submission is only scheduled for a review IFF it gets a review manager. It is not a position people queue for. :-) So, no one has come forward for impl_ptr to be a review manager... as I can see for other submissions also... Before such a manager-less submission would be put in the queue and stay on the radar... Now such a submission generates initial interest on the list, then drifts out of the scope and is left behind/forgotten. It's unfortunate.
RE: obscurity
You might consider going to https://github.com/yet-another-user/pimpl and adding a star to the project. It raises its visibility in a GitHub search with everything following.
RE: accepted into boost
Initially I personally had my doubts if it was not too simple, obvious and basic. Now Giel van Schijndel joined in and made immense contributions/improvements to all policies. Namely, 'unique' and 'copied' policies are std::unique_ptr-based and pimpl-objects are of the 'void*' size... no memory overhead!.. Hugely useful IMO. Then, for high performance two in-place (no dyn. mem. allocation) policies are really well-done with one such policy not having any mem. overhead at all. So, IMO the submission has certainly something to offer functionality-wise and deployment-wise beyond manual pimpl-idiom implementation and would be a useful addition to the existing set of smart pointers...
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman /listinfo.cgi/boost
On 2017-09-09 17:12, Richard Hodges via Boost wrote:
Hi Vladimir, great to see that good has come of the review process.
Well, to clarify -- there was no Boost review... There was some initial (and very useful) input to my submission request but Boost-wise the submission did not get anywhere... as the review mgr could not be found... so far anyway. Still, something good indeed has come of as after Giel's effort impl_ptr is a serious smart pointer for the purpose.
If you'll allow my first commit to add a CMakeLists.txt file I'll happily just start using it and contributing where possible.
All contributions are most and truly welcome. For starters I'd suggest you fork the project first into your GitHub userspace. Apply your changes. Then, submit a pull request, Giel or I will be happy to merge it in. I might not be able to do that for a few more days (I am away). Looking forward to your input.
On 8 September 2017 at 22:53, Vladimir Batov wrote:
On 2017-09-09 01:34, Richard Hodges via Boost wrote:
How is the review process moving on? I have a clear need for this in my code right now.
Would love to see it get protection from obscurity by being accepted into boost sometime soon.
Thank you for your interest and encouragement. Much appreciated.
RE: review
As for the actual review, then (unfortunately) there is no one. From my understanding the Boost review process has changed and now a submission is only scheduled for a review IFF it gets a review manager. It is not a position people queue for. :-) So, no one has come forward for impl_ptr to be a review manager... as I can see for other submissions also... Before such a manager-less submission would be put in the queue and stay on the radar... Now such a submission generates initial interest on the list, then drifts out of the scope and is left behind/forgotten. It's unfortunate.
RE: obscurity
You might consider going to https://github.com/yet-another-user/pimpl and adding a star to the project. It raises its visibility in a GitHub search with everything following.
RE: accepted into boost
Initially I personally had my doubts if it was not too simple, obvious and basic. Now Giel van Schijndel joined in and made immense contributions/improvements to all policies. Namely, 'unique' and 'copied' policies are std::unique_ptr-based and pimpl-objects are of the 'void*' size... no memory overhead!.. Hugely useful IMO. Then, for high performance two in-place (no dyn. mem. allocation) policies are really well-done with one such policy not having any mem. overhead at all. So, IMO the submission has certainly something to offer functionality-wise and deployment-wise beyond manual pimpl-idiom implementation and would be a useful addition to the existing set of smart pointers...
@Vladimir - issue posted on github. On 9 September 2017 at 09:52, Vladimir Batov via Boost < boost@lists.boost.org> wrote:
On 2017-09-09 17:12, Richard Hodges via Boost wrote:
Hi Vladimir, great to see that good has come of the review process.
Well, to clarify -- there was no Boost review... There was some initial (and very useful) input to my submission request but Boost-wise the submission did not get anywhere... as the review mgr could not be found... so far anyway. Still, something good indeed has come of as after Giel's effort impl_ptr is a serious smart pointer for the purpose.
If you'll allow my first commit to add a CMakeLists.txt file I'll happily
just start using it and contributing where possible.
All contributions are most and truly welcome. For starters I'd suggest you fork the project first into your GitHub userspace. Apply your changes. Then, submit a pull request, Giel or I will be happy to merge it in. I might not be able to do that for a few more days (I am away). Looking forward to your input.
On 8 September 2017 at 22:53, Vladimir Batov wrote:
On 2017-09-09 01:34, Richard Hodges via Boost wrote:
How is the review process moving on? I have a clear need for this in my
code right now.
Would love to see it get protection from obscurity by being accepted into boost sometime soon.
Thank you for your interest and encouragement. Much appreciated.
RE: review
As for the actual review, then (unfortunately) there is no one. From my understanding the Boost review process has changed and now a submission is only scheduled for a review IFF it gets a review manager. It is not a position people queue for. :-) So, no one has come forward for impl_ptr to be a review manager... as I can see for other submissions also... Before such a manager-less submission would be put in the queue and stay on the radar... Now such a submission generates initial interest on the list, then drifts out of the scope and is left behind/forgotten. It's unfortunate.
RE: obscurity
You might consider going to https://github.com/yet-another-user/pimpl and adding a star to the project. It raises its visibility in a GitHub search with everything following.
RE: accepted into boost
Initially I personally had my doubts if it was not too simple, obvious and basic. Now Giel van Schijndel joined in and made immense contributions/improvements to all policies. Namely, 'unique' and 'copied' policies are std::unique_ptr-based and pimpl-objects are of the 'void*' size... no memory overhead!.. Hugely useful IMO. Then, for high performance two in-place (no dyn. mem. allocation) policies are really well-done with one such policy not having any mem. overhead at all. So, IMO the submission has certainly something to offer functionality-wise and deployment-wise beyond manual pimpl-idiom implementation and would be a useful addition to the existing set of smart pointers...
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman /listinfo.cgi/boost
2017-09-08 22:53 GMT+02:00 Vladimir Batov via Boost <boost@lists.boost.org>:
RE: obscurity
You might consider going to https://github.com/yet-another-user/pimpl and adding a star to the project. It raises its visibility in a GitHub search with everything following.
Regarding visibility, maybe registering in Boost library Incubator ( http://blincubator.com/) should attract potential users and reviewers. Richard, Since you have expressed a clear interest in the library, maybe you could consider *endorsing* it, as required by the new Boost Library Submission process: http://www.boost.org/development/submissions.html This would be a hint for potential reviewers and review manager that the library is worth investing their time. Regards, &rzej;
Thanks for pointing this out Andrzej. I'll refactor another project of mine to use it and give it a thorough workout. R On 11 September 2017 at 09:04, Andrzej Krzemienski via Boost < boost@lists.boost.org> wrote:
2017-09-08 22:53 GMT+02:00 Vladimir Batov via Boost <boost@lists.boost.org
:
RE: obscurity
You might consider going to https://github.com/yet-another-user/pimpl
and
adding a star to the project. It raises its visibility in a GitHub search with everything following.
Regarding visibility, maybe registering in Boost library Incubator ( http://blincubator.com/) should attract potential users and reviewers.
Richard, Since you have expressed a clear interest in the library, maybe you could consider *endorsing* it, as required by the new Boost Library Submission process: http://www.boost.org/development/submissions.html
This would be a hint for potential reviewers and review manager that the library is worth investing their time.
Regards, &rzej;
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/ mailman/listinfo.cgi/boost
participants (5)
-
Andrzej Krzemienski -
Gevorg Voskanyan -
Richard Hodges -
Seth -
Vladimir Batov