Re: [lmi] Font-size oddity

[Vadim Zeitlin]

On Fri, 2 Nov 2018 13:26:43 +0000 Greg Chicares <address@hidden> wrote:

GC> so, IIUC, the diff is equivalent to:
GC> 
GC>   T* p = html_parser.GetProduct();
GC> - std::unique_ptr<T> u = std::unique_ptr<T>(new T(p));
GC> + std::unique_ptr<T> u {p};

 Yes. There are 2 differences here:

1. The old code used wxHtmlContainerCell copy ctor, which is "shallow" i.e.
   doesn't copy the cell contents (yes, this is pretty surprising and I
   didn't know about it until yesterday).

2. The old code leaked the pointer "p" (which is again somewhat surprising
   although here I must admit that I had known about it but just forgot
   this when writing the initial version of this code).

 It so happens that fixing (2) also obviates the need for (1) and so
resolves both problems at once.

GC> I thought wx managed the deletion of any pointers it returns to client
GC> code, so isn't it wrong to construct a unique_ptr from such a raw pointer?

 In this particular case, GetProduct() returns a pointer which should be
freed by caller. This is indeed unusual in wx API and this is why I made
the mistake (1) above.

 But the fact is that wxHtmlParser::GetProduct() (and Parse(), using it)
returns a new heap-allocated pointer.

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

 No, wxHtmlContainerCell is not a value-like class at all, it can only be
operated on via pointers.

GC> But if I see
GC>   T* t;
GC> I despair: I don't know anything about its ownership or lifetime.

 Usually you do: raw pointers can only be non-owning in reasonable C++
code. wxHtmlParser::GetProduct() is just not being reasonable but, in its
defense, std::unique_ptr<> hadn't existed yet in the bad old days when it
was written and even std::auto_ptr<> wasn't widely available.

 When dealing with legacy code, the best we can do is to store such owning
pointers in std::unique_ptr<> as quickly as possible, which is what the
code here tried to do it -- unfortunately not fully successfully.

GC> so why can't we just renounce pointers and feel groovy all the time,
GC> relying on move semantics to keep the cost zero?

 We could if we rewrote wxHTML from scratch. It just doesn't seem to be
worthwhile to do it.

GC> Is std::make_unique itself arguably defective?

 No, of course not. It behaves exactly as intended.

 It's wxHtmlContainerCell copy ctor which is very surprising but, again, I
don't think we can seriously consider changing it.


 Please let me know if these explanations help you to understand the
problem and its fix better or if you still have any unanswered questions,
VZ