Re: [lmi] Non-null smart pointer

[Greg Chicares]

On 13/10/2018 13.31, Vadim Zeitlin wrote:
> On Fri, 12 Oct 2018 23:18:31 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 10/12/18 11:06 AM, Vadim Zeitlin wrote:
[...]
> GC> > but perhaps we could write our own not_null<unique_ptr<T>> using the
> GC> > information above (and yes, I'd be willing to do this, please let me
> GC> > know if I should).
> GC> 
> GC> That's an intriguing idea. How would you design it? I see at least
> GC> two ways. One way (1) is to test whether a pointer is null whenever
> GC> it's dereferenced; another (2) is to test whether it would be made
> GC> null whenever it's initialized or assigned. But std::unique_ptr is
> GC> movable-from, so it wants to be nullable: is that the crucial
> GC> problem? and if so, is the solution to use a nonmovable scoped_ptr,
> GC> and avoid move semantics?
> 
>  We want to have a smart pointer that can be returned from a function, so
> it must be either copyable or movable. And we also want it to be owning,
> which means it can't be copyable (unless you impose the use of reference
> counting, i.e. make it shared_ptr<> instead of unique_ptr<>), so it has to
> be movable.

Does lmi need exactly one kind of smart pointer, or more than one?
Do all our smart pointers need to be returnable from a function,
or only some of them? Do any actually need reference counting?

I find this casual measurement surprising:

/opt/lmi/src/lmi[0]$grep unique_ptr *.?pp |wc -l  
31
/opt/lmi/src/lmi[0]$grep shared_ptr *.?pp |wc -l  
135

because I doubt that most of them need reference counting. Some
of this code was originally written with the limitations of a
16-bit compiler in mind; for example, I think the motivation
for the fourteen smart pointers in 'basic_values.hpp' such as
    std::shared_ptr<product_data>       ProductData_;
was to keep "large" objects out of a sixty-four-kilobyte stack
segment, so probably no smart pointers are really needed there
in this century.

If we sort all of lmi's smart pointers into buckets, I'm not
sure how many buckets we'd need, but I'm pretty sure that
the "no smart pointer needed at all" bucket is nonempty, and
I imagine that the "reference counting needed" bucket is
relatively small if not quite empty.

> GC> Either way, I guess we'd be transforming every raw pointer into a new
> GC> kind of smart pointer.

True iff the scope of this discussion is all lmi pointers
(even those that simply interface to C functions like strtod()?).
Not true if the scope is all lmi smart pointers, or, a little
more broadly, all our use of malloc() and 'new'.

> GC> there's a 'wxPdfDocument* whatever' line somewhere now; would we
> GC> have to wrap that, and then explicitly get() it prior to each use?

[I've restarted konsole and can grep again:]

pdf_writer_wx.cpp: wxPdfDocument* const pdf_doc = pdf_dc_.GetPdfDocument();

>  Yes, this would presumably become "not_null<wxPdfDocument*> whatever" and
> you'd have to use get() to retrieve the raw pointer. OTOH, as it is not
> supposed to be null, we shouldn't even need to use the pointer, only the
> reference, and this can be obtained via operator*(), just as with the raw
> pointer, so there would be no syntactic overhead.

void pdf_writer_wx::output_image
...
    // Use wxPdfDocument API directly as wxDC doesn't provide a way to
    // set the image scale at PDF level and also because passing via
    // wxDC wastefully converts wxImage to wxBitmap only to convert it
    // back to wxImage when embedding it into the PDF.
    wxPdfDocument* const pdf_doc = pdf_dc_.GetPdfDocument();
    LMI_ASSERT(pdf_doc);

    pdf_doc->SetImageScale(scale);
    pdf_doc->Image(image_name, image, x, pos_y);
    pdf_doc->SetImageScale(1);

That pointer needn't be copyable, moveable, or reference counted;
it doesn't even need ownership, so maybe something like this

  http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n3840.pdf
  "A Proposal for the World’s Dumbest Smart Pointer"

would be appropriate--of course, with non-null-ness added. The benefit
of using that almost everywhere, and raw pointers almost nowhere, is
that we can't forget to write an assertion, because it's done implicitly.

> GC> (I should think we'd automate dereferencing by defining operator.()
> GC> and operator->(), but if we want to pass a pointer (say, to pass a
> GC> wxWindow* to a wx function), then we'd need get() each time, or,
> GC> equivalently, operator()()).
> 
>  wxWidgets is problematic because it requires passing pointers everywhere
> and while in most places these pointers must be non-null, sometimes they
> may be null. I'm afraid there is no good solution here, other than
> providing a better API.

We aren't going to reimplement the wxWidgets API this decade, but we
could make changes like this:

+    nonnull_ptr<wxPdfDocument> const pdf_doc = pdf_dc_.GetPdfDocument();
-    wxPdfDocument* const pdf_doc = pdf_dc_.GetPdfDocument();
-    LMI_ASSERT(pdf_doc);

     pdf_doc->SetImageScale(scale);
     pdf_doc->Image(image_name, image, x, pos_y);
     pdf_doc->SetImageScale(1);

where the last three lines require no change.

> GC> Wouldn't either (1) or (2) alone suffice?

Recapitulation:
  (1) prevent dereferencing when pointer is null
  (2) prevent pointer from becoming null (unless moved from)

>  Unfortunately no. We want to do (2) in order to detect the errors (as
> assigning NULL to a not_null_ptr is an error) as early as possible. But we
> also have to do (1) because we can't statically prevent the use of a
> moved-from object. [...snip link to a fascinating Herb Sutter slideshow...]

Ah, yes, of course: we need both.

>  To summarize, we can have not_null_ptr<> satisfying our needs, but with
> some run-time overhead. In practice I think gcc should optimize away the
> checks in most cases, but probably not always.

But that's okay. "Jump if zero" never was a really slow instruction,
even before CPUs were pipelined. It's kind of like your practice of
writing at() by default instead of operator[](): the overhead is
minuscule.

I'm just puzzled that the GSL not_null<> documentation claims to
improve runtime speed compared to coding explicit assertions to
prevent (1) manually. I don't see how they could claim that. But
it doesn't really matter.