Re: [lmi] Reference lifetime extension

[Greg Chicares]

On 10/12/18 11:06 AM, Vadim Zeitlin wrote:
> On Fri, 12 Oct 2018 01:17:06 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 10/11/18 11:58 PM, Vadim Zeitlin wrote:
> GC> > On Thu, 11 Oct 2018 23:29:12 +0000 Greg Chicares <address@hidden> wrote:
[...]
> GC> > Getting rid of shared_ptr<> is a good thing (and I admit that I have
> GC> > absolutely no idea why had I used it here in the first place),
> GC> 
> GC> I suppose you copied the progress_meter callback mechanism for
> GC> group quotes, and then transitively for PDF illustrations.
> 
>  Yes, this looks like a likely explanation. Whenever I can find something
> similar in the existing code base (with the exception of its legacy parts),
> I try to make new code as similar to the existing lmi code as close to it
> as makes sense.
> 
> GC> > but I would have preferred just replacing it with unique_ptr<>
> GC> > instead of completely removing ledger_pdf_generator class.
> GC> 
> GC> Would unique_ptr<> work for progress_meter, and, similarly,
> GC> the "group_quote_pdf_gen" classes? Class progress_meter is so
> GC> old that perhaps the only smart pointers available at the time
> GC> were boost::shared_ptr and std::auto_ptr.
> 
>  AFAICS unique_ptr<> would work perfectly fine for both of them. But I
> still wonder why had shared_ptr<> been used originally, even auto_ptr<>
> would have worked for this.

It was checked into CVS on 2005-04-19, from an older original; I can't
remember the rationale for choosing shared_ptr.

> GC> I'd much rather use no smart pointers at all,
> 
>  I think unique_ptr<> is perfectly fine and have absolutely no qualms about
> using it. This is not the case for shared_ptr<>, but for reasons which have
> nothing to do with it being possibly null (rather, it's because it
> introduces non-local relationships in the code that make it difficult to
> reason about, just like global variables).

Yes, I'm doubly concerned about shared_ptr for that reason.

> GC> because pointers can be null and it's tiresome to check that they
> GC> aren't. Is it even possible to write "smart reference" classes that
> GC> can't ever contain a null value?
> 
>  Not without sacrificing movability because being null is the only
> reasonable possibility for an object that was just moved from. If you don't
> need movability, then you could have some non_null_scoped_ptr, but in
> practice it's not very useful because you can just replace it with an
> object instead and you can't neither initialize it conditionally nor return
> from a function.
> 
>  The core guidelines recommend using not_null<T*> which should appeal to
> you as it avoids any mention of smart pointers (which still need to own the
> object, however). I don't like it that much because if you accept the
> guideline that functions not changing ownership should accept pointers only
> as T*, it means having a lot of .get() calls in the code, but maybe you
> wouldn't mind it that much.

I can't really say, without seeing how obtrusive it would be. And it
seems really difficult to find usage examples online; is that because
it's not actually in broad use?

>  Finally, there is a natural idea of using not_null<unique_ptr<T>> but
> there seems to be a problem with this in the most widely implementation of
> not_null, see https://github.com/Microsoft/GSL/issues/89 and the proposed
> fix at https://github.com/Microsoft/GSL/pull/675

It sounds like an attractive idea. But if I'm reading this correctly,
issue 89, opened 2015-09, is a pretty shocking shortcoming; and PR 675
was proposed 2018-05, but hasn't been accepted or rejected yet. I'm led
to question whether the library is actively maintained, or even sound;
but perhaps designing this particular library facility to embrace smart
pointers is just surpassingly difficult.

>, but perhaps we could write our
> own not_null<unique_ptr<T>> using the information above (and yes, I'd be
> willing to do this, please let me know if I should).

That's an intriguing idea. How would you design it? I see at least
two ways. One way (1) is to test whether a pointer is null whenever
it's dereferenced; another (2) is to test whether it would be made
null whenever it's initialized or assigned. But std::unique_ptr is
movable-from, so it wants to be nullable: is that the crucial
problem? and if so, is the solution to use a nonmovable scoped_ptr,
and avoid move semantics?

Either way, I guess we'd be transforming every raw pointer into a new
kind of smart pointer. I've just experienced another konsole krash,
so I kan't kopy kode examples with the X klipboard, but for example
there's a 'wxPdfDocument* whatever' line somewhere now; would we
have to wrap that, and then explicitly get() it prior to each use?
(I should think we'd automate dereferencing by defining operator.()
and operator->(), but if we want to pass a pointer (say, to pass a
wxWindow* to a wx function), then we'd need get() each time, or,
equivalently, operator()()).

Wouldn't either (1) or (2) alone suffice? Does the GSL implementation
however do both? The documentation says its purpose is:
| To help avoid dereferencing nullptr errors. To improve performance
| by avoiding redundant checks for nullptr.
which seems to imply that it does only (2), and (2) costs less than
(1); but in the implementation:
  https://github.com/Microsoft/GSL/blob/master/include/gsl/pointers
I see
    Expects(ptr_ != nullptr);
in both the ctors and in get(), so I don't see how this can improve
performance. Maybe I just haven't understood it well enough.