Re: [lmi] Font-size oddity

[Greg Chicares]

On 2018-11-02 02:02, Vadim Zeitlin wrote:
[...]
> GC> Can you fix this anomaly?
> 
>  Yes, please see https://github.com/vadz/lmi/pull/102

Well, it fixes the anomaly, so I've committed and pushed it, but
I don't understand why the unpatched code did the wrong thing.
(Maybe I could have understood if it had crashed, but I can't
see how it could legitimately behave in an unintended way.)

>  The commit message explains what's going on
| make_html_from() function didn't do anything useful as it made a shallow
| copy of the new document it created (leaking the original one in the
| process) instead of simply taking ownership of it.
|
| This meant that the object returned from this function did not contain
| the color and font cells which was the sole reason for its existence in
| the first place and, due to this, the header cells created using this
| function didn't use the correct initial font, resulting in visibly wrong
| appearance, which is fixed by this change.

-    auto document_cell = std::make_unique<wxHtmlContainerCell>
-        (static_cast<wxHtmlContainerCell*>(html_parser.GetProduct())
-        );
+    // Take ownership of the DOM containing just the initial colors and font.
+    std::unique_ptr<wxHtmlContainerCell> document_cell
+        {static_cast<wxHtmlContainerCell*>(html_parser.GetProduct())
+        };

make_unique does this:
  unique_ptr<T>(new T(std::forward<Args>(args)...))
so, IIUC, the diff is equivalent to:

  T* p = html_parser.GetProduct();
- std::unique_ptr<T> u = std::unique_ptr<T>(new T(p));
+ std::unique_ptr<T> u {p};

I thought wx managed the deletion of any pointers it returns to client
code, so isn't it wrong to construct a unique_ptr from such a raw pointer?
When the unique_ptr goes out of scope, its deleter deletes the object, and
then wx later deletes the object, causing UB, right?

Also, I'm supposing that html_parser.GetProduct() returns a pointer to
some object that's created and managed by wx, and that it continues to
exist as long as the client needs it, and is later cleaned up by wx.
In that case, it seems to me that a shallow copy of such a pointer
would just do the right thing: it would have a font_type* pointer that
would point to an original font object, instead of to a (deep) copy
of that font object, but both would embody the same font, so we'd
be okay, right?

However, the context is:

    wxHtmlWinParser html_parser;
    initialize_html_parser(html_parser);
    html_parser.InitParser(wxString{});
[this is the original code, before the patch]
    auto document_cell = std::make_unique<wxHtmlContainerCell>
        (static_cast<wxHtmlContainerCell*>(html_parser.GetProduct())
        );

The wxHtmlWinParser object is created on the stack. Does that mean
that GetProduct returns a pointer to some temporary entity that lives
only while the parser object lives? Is it as though that object contains
  font_type*  the_font;
  color_type* the_color;
so that after pdf_writer_wx::make_html_from() returns, whatever those
pointers originally pointed to has been deleted, and the pointers are
left dangling? In such a case, using them later would be UB. Is that
what happened? I.e., were we dereferencing dangling pointers that
just happened to point to...some part of memory that somehow was
interpreted as a real font and a real color that no longer meant the
font and color intended?

Or did we instead have a pointer to a legitimate object containing
  font_type*  the_font;
  color_type* the_color;
which represented a valid font and a valid color, but not the font
and color we wanted? If so, then does that mean that the
wxHtmlContainerCell copy ctor doesn't perform a deep copy, which
could be considered a shortcoming? IOW, assuming the original code
was equivalent to:
  std::unique_ptr<T> u = std::unique_ptr<T>(new T(p));
did 'new T(p)' give us a valid wxHtmlContainerCell which represented
a different color and font than the original from which is was cloned?
(I'm guessing that's what happened, but wondering why.)

Given that the wxHtmlWinParser was created on the stack, and that
GetProduct() returns a wxHtmlContainerCell* in the guise of a
wxObject*, couldn't we just dereference that wxHtmlContainerCell*
and then return it by reference, avoiding pointers and the issues
that come with them? E.g.:

wxHtmlContainerCell pdf_writer_wx::make_html_from(wxHtmlCell* cell)
 {
...
    wxHtmlWinParser html_parser;
...
    wxHtmlContainerCell z {*html_parser.GetProduct()};
...
    return z;
}

or maybe

wxHtmlContainerCell&& pdf_writer_wx::make_html_from {...return std::move(z);}

IOW, if I see
  T t;
then that's clearly an object that I own, and I can reliably
pass it elsewhere, copying or moving it as I please. But if I see
  T* t;
I despair: I don't know anything about its ownership or lifetime.
As Simon & Garfunkel observe:
  I have no need of pointers--pointers cause me pain:
  It's undefined behavior I disdain...
so why can't we just renounce pointers and feel groovy all the time,
relying on move semantics to keep the cost zero?

Is std::make_unique itself arguably defective? I stared at the old
and new code for a couple minutes and at first couldn't see why
they weren't thoroughly equivalent. If I say
   auto z = std::make_unique<T>(T* p)
then, perhaps naively, I anticipate that I'll get a smart pointer
that holds the raw pointer I provided, IOW
  unique_ptr<T>(p)
instead of
  unique_ptr<T>(new T(std::forward<Args>(args)...))
which I guess means
  unique_ptr<T>(new T(p))
That is, if I had specified constructor arguments:
   auto z = std::make_unique<T>(T(arg1, arg2)
then naturally I must be asking for a T to be constructed from
arg1 and arg2, but if I pass a pointer to an new object I've just
constructed, then constructing a new object from that new object
seems to violate the principle of least surprise.