Re: out_ptr review

From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2019-06-29 21:29:10

sob., 29 cze 2019 o 21:20 JeanHeyd Meneide via Boost <boost_at_[hidden]>
napisał(a):

> Dear Phil Endecott,
>
>      Thank you for your review.
>
>      There has been a consistent view that there should be beautiful,
> hand-written wrappers around all of the C functions that you or your
> application care about. I am convinced these wrappers are both
> fundamentally flawed and wrong for users.
>
>      Let's look at your Frambuffers example:
>
> class Framebuffer: boost::noncopyable
> {
>   GLuint handle;
> public:
>   Framebuffer() { glGenFramebuffers(1, &handle); }
>   ~Framebuffer() { glDeleteFramebuffers(1, &handle); }
> };
>
> To start, boost::out_ptr::out_ptr accomodates non-pointer handles, the same
> as std::unique_ptr accommodates them. Your code can be rewritten using a
> std::unique_ptr<GLuint, frame_buffer_deleter>:
>
> struct gl_handle {
>      GLuint handle;
>
>      friend bool operator ==(GLuint, nullptr);
>      friend bool operator ==(nullptr, GLuint);
>      friend bool operator ==(GLuint, GLuint);
>      friend bool operator !=(GLuint, nullptr);
>      friend bool operator !=(nullptr, GLuint);
>      friend bool operator !=(GLuint, GLuint);
> };
> struct frame_buffer_deleter {
>      using pointer =  gl_handle;
>
>      void operator( gl_handle handle){
>           glDeleteFramebuffers(handle.handle, 1);
>      }
> };
>
> class Framebuffer
> {
>   std::unique_ptr<GLuint, frame_buffer_delete>;
> public:
>   Framebuffer() { glGenFramebuffers(1,
> boost::out_ptr::out_ptr<GLuint>(handle)); }
> };
>
> "That's so much more code, why would anyone do that?!" you ask. For the
> following reasons:
>
>    - There's no more destructor to write.

I must admit, I do not understand this argumentation. No destructor to
write, but instead a deleter to write. At least destructor is in one place
with the constructor.

If this class ever got marginally
>    more complicated than just be a specifically-named unique_ptr,
> allocation
>    of the framebuffer may succeed but an exception can be thrown, locking
>    driver resources because you never deleted them:
>    http://coliru.stacked-crooked.com/a/910aa40650884efb and
>    http://coliru.stacked-crooked.com/a/48126c474d033c5d both demonstrate
>    that. There is a reason why the rule of zero -- by R. Martinho Fernandes
>    and included in Scott Meyers's book and spoken about around the world --
>    became so popular and important in C++11.
>

Of course, if somebody later comes in, and starts adding members and
functionality to this class it will get broked. C++ does not prevent users
from conciously dong nasty things. But the point of such class would be to
only manage a single resource. With this single responsibility in mind
no-one will be adding function calls to the class to "improve" it.

   - Reusable: right now I have programmed to specifically write to your
>    framebuffer example. The code I wrote to do this in reality (and ported
> to
>    multiple platforms, including for Vulkan and DirectX) used unique_ptr to
>    hold the handle and I could make it handle creating more than 1
> framebuffer
>    by adding a sized deleter. Note that such a deleter makes it a strong
> type:
>    unique_ptr<GLuint[], sized_deleter> != unique_ptr<GLuint,
> regular_deleter>.
>

Now it looks like you have to define another deleter.

   - Your code hides it in the internals of Framebuffer and hard-codes it:
>    if you were to ever change the class, you would need to change the ctor,
>    dtor, and anywhere else that the assumption takes place, or write an
>    entirely new class "Framebuffer*s*". If written in the same style of
>    your other one, it would be vulnerable to the same problems.
>

If I suddenly change my mind and instead of one resource, I have to manage
a list of resources, this is hardly surprising that I will need a new type.
I will then have an aggregate, like std::vector<Framebuffer>, and each
Framebuffer will be only managing a single resource. I can see no way the
problem you describe with leaking resources will sneak in. Doesn't seem
more complicated then writing a yet another deleter and using unique_ptr
for managing non-pointers.

   - Bonus Points: boost::noncopyable is no longer needed, just the unique
>    pointer which already renders the class move-only.
>

This does not seem a sufficient motivation to switch to out_ptr().

> There are similar problems with your PNG example.
>
>      All of this goes to show that while you can write cute one-off
> examples that seem simpler, they do not scale to the fundamental reality
> that is handing resources, dealing with exceptions, and more in C++ APIs.
>

What "scaling" do you mean here? I have seen one example of changing from
managing one resource to managing a list of resources, which does not fit
into my definition of "scaling". Building classes that have a single
responsibility of managing a single resource does address the problem of
dealing with exceptions and early returns. Also, I would like to know what
you mean by "and more" in this claim.

out_ptr scales far beyond what you just wrote, handles the fact that
> someone can change their pointer type from `handle` to `handle[]` for
> multiple resources, and more.

Do you mean by "scaling" changing for m propagating a single resource to
propagting a list of resources? How often do you do that? (Given that you
have changed the type and now you have to change every place that uses the
code.)

A small degree of additional templating

Well, "additional templating" doesn't sound as something good.

-- if
> you have the time and skill for it --

What skills are required here?

can also make this example even more
> generic and scale it far beyond what you have written in your example.

Again, what do you have in mind whan you say "scale" here? Can you show us
an example (and the reason for doing this "scaling")?

And
> boost::out_ptr would require no additional changes to its usage to adapt.
>

>From what I understand so far, maybe out_ptr would not require additional
changes, but the code that uses out_ptr would.

>      The rest of your review comments have already been addressed in the
> feature/boost.review branch and have been answered in other out_ptr
> threads. This API has been used with libavformat, Python's C API, OpenGL,
> DirectX, COM interfaces, and more. Previous iterations of this class have
> been used in VMWare driver infrastructure, all of Microsoft WRL library (as
> an improvement over its original CComPtr), and more (see the proposal p1132
> <https://thephd.github.io/vendor/future_cxx/papers/d1132.html#motivation>,
> which details previous iterations of this). It has also been written by
> Raymond Chen, in his pursuit for a cleaner interface, but with some issues
> regarding destructor order for temporaries in function calls in C++ (which
> were correctly called out in the paper and the documentation before he
> wrote the article). See:
> https://devblogs.microsoft.com/oldnewthing/20190429-00/?p=102456
>
>      Finally, I have been vending this on my own time at no cost to
> individuals and companies for over a year, starting from its earlier
> versions from a (now superseded) repository:
>
> https://groups.google.com/a/isocpp.org/d/msg/std-proposals/8MQhnL9rXBI/FaK1RZNnCwAJ
> .
> I was also not the one to make this idea in the first place: I was taught
> the technique  over 4 years ago by Mark Boyall in a C++ chat room.
>
>      While I appreciate your criticisms, I do not think your examples and
> suggestions scale to Fortune 500 company code base infrastructure or small
> individual hobby codebases.

Again, I do not know what "scaling" do you mean. From what you are saing I
gather that a number of projects found it useful, but what does this have
to do with "scaling"?

The way I was always taught what "to scale" in the context of software
developement mean is that when you put N times more resources to your
system, the program will run N times more efficient -- as opposed to
nonscalable programs, where if you put N times more resources you will not
get any (or minimum) performance gain.

Regards,
&rzej;

out_ptr, however, does and has, and should be
> included in the Standard. And maybe Boost, too.
>
>      Thank you for your time.
>
> Sincerely,
> JeanHeyd Meneide
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>