wt-4 changes

Added by Gunnar Skogsholm over 1 year ago

The wt-4 changes have been difficult to integrate with. It's sometimes much more difficult to figure out the offending line of code, because compiler errors are pointing to Wt classes, even though they build fine on their own.

Just my opinion, but I don't think it was necessary to make such a large change to the interface. Apparently, there was a good reason for Wt to take complete control of object life cycles? Perhaps it would have been more straightforward just to add additional functions?

"here look at this object and make a copy if you must" -> const Type&
"Here this object is yours" -> Type&&

Anyways, water under the bridge, but this might actually be a design bug.

These lines of code had to change from:
treeNode = new Wt::WTreeNode( Name );
parentNode->addChildNode( treeNode );
treeNode->label()->clicked().connect( this, &Branch::click );

to:
//treeNode = new Wt::WTreeNode( Name );
treeNode = parentNode->addChildNode(make_unique<Wt::WTreeNode>(Name));
treeNode->label()->clicked().connect([this](const Wt::WMouseEvent& e) { this->click(e); });

The 3.3.7 connect methods were better. Did I miss something? Otherwise, it was insisting that my class Branch have some relationship to Core::observable.


Replies (13)

RE: wt-4 changes - Added by Roel Standaert over 1 year ago

Wt has always worked this way: the parent always owned the child and the child would be deleted along with the parent. Of course, back then you could indeed call delete on a child widget without taking ownership first, and Wt would do the right thing. For Wt 4, we made it more clear: raw pointers are always non-owning. In many cases (like the children of WContainerWidget), you can take ownership with removeChild if you want to, and then the lifetime of the child can still be managed independently from its position in the widget tree.

We made these changes to make the memory model more obvious, like Herb Sutter and Bjarne Stroustrup's Core Guidelines recommend.

Personally, I don't have a problem with changing this:

treeNode = new Wt::WTreeNode( Name );
parentNode->addChildNode( treeNode );

To this:

treeNode = parentNode->addChildNode(make_unique<Wt::WTreeNode>(Name));

I don't know if you have a problem with that? In the former case, the raw pointer treeNode changes from owning in the first line to non-owning in the second line.

If you make Branch inherit from Wt::Core::observable or Wt::WObject that line should work:

treeNode->label()->clicked().connect( this, &Branch::click );

This is to guarantee that when this is deleted, the slot is automatically disconnected, so that this->click won't be called. Of course, if you can know that treeNode->label() will always be deleted before this, the lambda can be used, but is a bit more verbose.

I guess that in theory it could be possible to also make that line work without the safety mechanism of Wt::Core::observable/Wt::Core::observing_ptr, but I'm not quite sure what the right choice would be there.

It's sometimes much more difficult to figure out the offending line of code, because compiler errors are pointing to Wt classes, even though they build fine on their own.

In which situations is this the case? I could see if something can be done to clarify these situations.

Regards,
Roel

RE: wt-4 changes - Added by Gunnar Skogsholm over 1 year ago

I don't know if you have a problem with that?

I'm not going to belabor the point by explaining my previous suggestion. You and your colleagues are creating a work of art, and I won't quibble about the details.

I guess that in theory it could be possible to also make that line work without the safety mechanism

This is what works for other parts of Wt:

        comboBoxWnd->activated().connect( std::bind( &ComboBox::write_model, this ) );

But when I try:
    //treeNode->label()->clicked().connect([this](const Wt::WMouseEvent& e) { this->click(e); });// this, &Branch::click );
    treeNode->label()->clicked().connect(std::bind(&Branch::click, this));

I get:
4>BranchWeb.cpp
4>C:\Dev\Couloir\3pLibs\wt-4\src\Wt/Signals/signals.hpp(418): error C2664: 'Wt::Signals::Impl::Connection Wt::Signals::Impl::ProtoSignal<E>::connect(const std::function<void (E)> &,const Wt::Core::observable *)': cannot convert argument 1 from 'const std::_Binder<std::_Unforced,void (__cdecl Couloir::Web::Branch::* )(const Wt::WMouseEvent &),Couloir::Web::Branch *const >' to 'const std::function<void (E)> &'
4>        with
4>        [
4>            E=Wt::WMouseEvent
4>        ]
4>C:\Dev\Couloir\3pLibs\wt-4\src\Wt/Signals/signals.hpp(418): note: Reason: cannot convert from 'const std::_Binder<std::_Unforced,void (__cdecl Couloir::Web::Branch::* )(const Wt::WMouseEvent &),Couloir::Web::Branch *const >' to 'const std::function<void (E)>'
4>        with
4>        [
4>            E=Wt::WMouseEvent
4>        ]
4>C:\Dev\Couloir\3pLibs\wt-4\src\Wt/Signals/signals.hpp(418): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
4>c:\dev\couloir\3plibs\wt-4\src\wt\WSignal(721): note: see reference to function template instantiation 'Wt::Signals::connection Wt::Signals::Impl::connectFunction<F,E>(Wt::Signals::Signal<E> &,const std::_Binder<std::_Unforced,void (__cdecl Couloir::Web::Branch::* )(const Wt::WMouseEvent &),Couloir::Web::Branch *const > &,const Wt::Core::observable *)' being compiled
4>        with
4>        [
4>            F=std::_Binder<std::_Unforced,void (__cdecl Couloir::Web::Branch::* )(const Wt::WMouseEvent &),Couloir::Web::Branch *const >,
4>            E=Wt::WMouseEvent
4>        ]
4>BranchWeb.cpp(40): note: see reference to function template instantiation 'Wt::Signals::connection Wt::EventSignal<Wt::WMouseEvent>::connect<std::_Binder<std::_Unforced,void (__cdecl Couloir::Web::Branch::* )(const Wt::WMouseEvent &),Couloir::Web::Branch *const >>(const F &)' being compiled
4>        with
4>        [
4>            F=std::_Binder<std::_Unforced,void (__cdecl Couloir::Web::Branch::* )(const Wt::WMouseEvent &),Couloir::Web::Branch *const >
4>        ]
4>BranchWeb.cpp(40): note: see reference to function template instantiation 'Wt::Signals::connection Wt::EventSignal<Wt::WMouseEvent>::connect<std::_Binder<std::_Unforced,void (__cdecl Couloir::Web::Branch::* )(const Wt::WMouseEvent &),Couloir::Web::Branch *const >>(const F &)' being compiled
4>        with
4>        [
4>            F=std::_Binder<std::_Unforced,void (__cdecl Couloir::Web::Branch::* )(const Wt::WMouseEvent &),Couloir::Web::Branch *const >
4>        ]

RE: wt-4 changes - Added by Velvet Jones 11 months ago

Anyways, water under the bridge, but this might actually be a design bug.

I'd like to agree with Gunnar about the large set of changes in Wt 4. And my feeling when I started a test port (didn't work out well at all) is that although you made explicit what was before implicit concerning ownership, a much better approach would have been to re-think ownership altogether, paying specific attention to how to make porting easier. If you had opted for a shared pointer class we could have replaced our raw pointers with your shared pointer, then the compiler would have issued an error anywhere we may have been attempting to explicitly delete the shared pointer class. I really don't think circular references are that difficult to solve, (had a dev do it in our own code), and a shared pointer would have been MUCH easier to integrate into existing code bases.

I'm a fan of what you're trying to accomplish here, and I also know what it's like to have tons of people with opinions about your software. But I want to point out some things just so all of you there understand the point of view of your (paying) customer:

1. I manage 500k+ lines of C++ code, of which about 15k are for a Wt frontend. I don't have lots of time to spend just on the Wt part of the code. Put another way, you're not the only partner I'm dancing with.

2. As much as I love the new features in C++ 11, 14, etc, in order to use them I have to upgrade my compilers on all machines/OSes I run builds on. I'm aware that 3.x didn't upgrade to c++11, and you've provided backwards compatibility for the 14 and 17 features you use. But consider this when you are eyeing that new great feature. As it stands now RHEL 6 is not EOL, and it cannot compile Wt 4 without deviating from the stock installation. That's not your fault, but that's what I face. What all of this means is that I am limited in my ability to port to Wt 4, and until I upgrade all my systems, I am concerned that my code base is getting way out of date, requiring too many changes when I can finally start the port.

3. Documentation, which I know is boring and tedious, needs to improve.

I get it. Wt is a young project and a great idea and it's a ton of fun to work in an environment like that. But this is a big change, and some of these changes feel like they're change for the sake of change.

And lastly - again - I think unique pointers were a really big mistake in Wt. And Roel, if you're quoting Herb Sutter and Bjarne Stroustrup, you've missed the point entirely. There is a usability problem with the Wt 4 interface.

RE: wt-4 changes - Added by Koen Deforche 10 months ago

We understand your point and your perspective. We also have large legacy projects built with Wt3 that haven't migrated nor is a migration already planned. Nor do we see this right now as a problem. We are already supporting Wt3 and Wt4 alongside for about a year and haven't run into issues nor are we expecting to run in issues soon. We don't know how long we will keep supporting both but we will certainly give an advance notice, and paying customers certainly get their say.

But at the same time (paying) customers had projects where they already adopted new c++11 pointers, and were working around Wt's deficiencies of not having them. It is also based on their feedback that we designed the Wt4 API with unique_ptr's. We did choose unique_ptr's because we believe it's most appropriate, not to annoy users...

I don't understand your point about code that is way out of date requiring too many changes when you start the port. I fail to see how a postponed port to c++11, as this suggests, would look different from the current port, and thus require the same amount of porting work? With Wt4 we are committed to stable APIs, just like we did for previous major releases (such as Wt3), and thus porting effort is not affected depending on your timing.

RE: wt-4 changes - Added by Velvet Jones 10 months ago

I don't have a problem with the ownership policy of Wt as much as I have a problem with how the ownership policy is presented. After reading a little about unique_ptr, I noticed that current general practice is to reference an object through either its unique_ptr or its raw pointer. This practice seems to me to have a scalability problem, since I can't just look at the raw pointer type and deduce how I should handle it. What I first saw when looking at the Wt API was a mishmash of unique_ptr and raw pointer, which appeared to clash. Although they do not clash (I did not claim the API was 'wrong'), they also do not make it easy to differentiate ownership at all points in my code. Of all the places I see a raw pointer in my code, there is only ever one place where I can know for sure the ownership policy, and that's where it's transferred to Wt. This is a less than optimal design because usability and code readability suffers.

As a concrete example, suppose the Wt API had returned, instead of raw pointers that I do not own, something like Wt::WPointer<T>, where T was a pointer with overloaded reference/pointer operators. Calls to delete this object would cause a compiler error, which would automatically enforce the fact that I don't own it. Further, my compiler would have told me every place in the API where I needed to change from raw pointer to the templated type above.

This is also a question of me finding the documentation as well as learning these new C++ features. There's nothing stopping me from assigning these pointers to my own template class as long as I (not my compiler, unfortunately) hunt down every place where such pointers are returned by Wt.

I was happy to see the effort taken to ensure that Wt4 only requires C++11. So my point about porting was just a reminder - that bumping that target up to C++14, for example, is not without large costs on my end.

RE: wt-4 changes - Added by Gunnar Skogsholm 10 months ago

Velvet,

For the record, I was having trouble with the connect function. However, part of my problem was understanding that function pointers in modern c++ aren't as straight-forward as I had expected. All good on that front.

I felt compelled to respond because you have loosely associated your comments with me by implication, and the authors are naturally very diplomatic.

I think unique pointers were a really big mistake in Wt.

unique_ptr indicates ownership, so this was the correct design choice.

quoting Herb Sutter and Bjarne Stroustrup, you've missed the point entirely.

Bjarne is a software god, and Herb Sutter is his prophet... :)

There is a usability problem with the Wt 4 interface

I disagree.

since I can't just look at the raw pointer type and deduce how I should handle it.

If you called "new", then you must called "delete". Even in c++98, this is considered a "best practice". If you're calling delete on raw pointers you didn't call "new" on, that's not good.

See #3 of https://herbsutter.com/2013/06/05/gotw-91-solution-smart-pointer-parameters/

that bumping that target up to C++14

I'm not aware of any major compiler that does not support c++17. If you're using c++11 in 2018, then you should probably use the Wt 3.x library. I for one, compile with msvc and clang, with the language setting at c++17.

RE: wt-4 changes - Added by Velvet Jones 10 months ago

I felt compelled to respond because you have loosely associated your comments with me by implication, and the authors are naturally very diplomatic.

I have and you are right, I should not be making any assumptions. Your initial critique that Wt had taken control of lifecycles wasn't correct of course, as Koen pointed out - it has always owned the objects you pass it. And it's possible that my own issues arise because it wasn't clear enough in Wt3 who owned what pointer.

As for unique_ptr indicating ownership, yes, but it seems to me that usability would have been much better had they been shared pointers. Now we pass in one type and get back another, and the type we get back looks just like a type that in C++98 we'd own. So IMO it would be clearer to either pass in a shared pointer and keep a copy of it around (since we so often need those pointers), or passing a unique_ptr and getting back something that says "you can't even try to delete this".

If you called "new", then you must called "delete". Even in c++98, this is considered a "best practice". If you're calling delete on raw pointers you didn't call "new" on, that's not good.

Except when using Wt4, because now I call new and then never call delete because of unique_ptr. The problem I am pointing out is that now I get back a raw pointer and I may very well think I need to delete it. It certainly looks like a memory leak at first glance because of the data type. Imagine that in my existing code base I've called delete on a raw pointer that Wt owns (precisely because the ownership policy was not clear to me in the Wt3 API). Having a different return type that I cannot call delete on would ensure that my code never compiles, and the error is easily caught.

I'm not aware of any major compiler that does not support c++17. If you're using c++11 in 2018, then you should probably use the Wt 3.x library. I for one, compile with msvc and clang, with the language setting at c++17.

The g++ version that shipped with RHEL 6 seems to barely support C++11 according to my research. Of course newer versions support it, but I may not have the ability to switch out the compiler on a certified build system.

For the record, I am not here to slam the Wt developers. I acknowledge that they're extremely capable developers and I'm a huge fan of Wt. If I did not care about the usability of this product I wouldn't bother to voice an opinion.

- Bud

RE: wt-4 changes - Added by Gunnar Skogsholm 10 months ago

Ok, we're good.

Confused why you think shared_ptr would be better since that's sending a message that is opposite of reality.

<blockquote>Except when using Wt4, because now I call new and then never call delete because of unique_ptr. The problem I am pointing out is that now I get back a raw pointer and I may very well think I need to delete it</blockquote>

Calling "new" isn't correct. Here is what I'm doing:

checkBoxWnd = parentWnd->addWidget(make_unique&lt;Wt::WCheckBox&gt;());

According to the previous Herb Sutter link, it's proper to provide the raw pointer for accessing functions. No assumption should be made about ownership of raw pointers. I would suggest reading that link again.

RE: wt-4 changes - Added by Velvet Jones 10 months ago

Read the link, interesting. I heartily agree with "Earwicker" and "CS" in the comments, they voice the same basic concerns I've stated here. Sutter's advice about passing shared pointers by reference is the equivalent of navel gazing. It's technically correct, but it will only cause development scalability problems in a large code base. I agree with Earwicker that it's better to write correct code first and optimize later. And "CS"'s comment is spot on - a declaration of a non-owning pointer would make them much easier to work with in real-world projects.

Thanks for the code snippet!

RE: wt-4 changes - Added by Gunnar Skogsholm 10 months ago

I disagree with your comment regarding Sutter's advice.

Is this a modern “Non-owning raw * pointer”? or is this a legacy “bad_sink” that should be updated

This is really not about the language at all. It's about refactoring legacy code. The only valid answer is that it's up to author. In C++98, a raw pointer was just that and did NOT imply ownership of any kind. In C++17, the same is true. Continuity has been preserved. CS doesn't seem to understand that.

It would be very nice if the language had a feature that allowed functions to explicitly state “pass me a non-owning pointer”. As it is now all they can say is “trust me, do I look like I might delete your non-owning raw pointer?”

I've got 15+ years of experience with c++ and 15+ with C#. For me, this comment is equivalent to "why can't C++ become C#?" My response to CS: Why don't you use C#?

Sutter's advice makes perfect sense to me: although we want to improve the language, we don't need or want to change the fundamental meaning of a pointer.

For example, anyone who feels strongly about this are free to use the encapsulation features of the language:

   class Better
   {
      private:
         std::unique_ptr<Good> unique;
      public:
         Better() { unique = std::make_unique<Good>(); }
         Good& pass_me_a_non_owning_ref() const { return *unique.get(); }
   };

   TEST( SmartPointers, pass_me_a_non_owning_ref )
   {
      // Setup
      Better Now;
      int e = 5;

      // Software Unit Under Test
      auto& TrustMeDoILookLikeIMightDeleteYourNonOwningRawPtr = Now.pass_me_a_non_owning_ref();
      e = TrustMeDoILookLikeIMightDeleteYourNonOwningRawPtr.e_value();

      // Analysis
      auto malicious = &TrustMeDoILookLikeIMightDeleteYourNonOwningRawPtr;

      // Evaluate
      try
      {
         delete malicious;
      }
      catch( ... )
      {
         FAIL() << "simply crazy";
      }
      EXPECT_EQ( e, 0 );

      // Cleanup
   }

However, other people have had similar thoughts as CS: http://boost.2283326.n4.nabble.com/Is-there-any-interest-in-non-owning-pointer-like-types-td4691421.html

Which appears to have resulted in: http://en.cppreference.com/w/cpp/experimental/observer_ptr

Problem is that when people take a close look at it, they realize: it's just for self-documentation
https://stackoverflow.com/questions/31862412/use-of-observer-ptr

Easier just to add a comment...

RE: wt-4 changes - Added by Velvet Jones 10 months ago

In C++98, a raw pointer was just that and did NOT imply ownership of any kind.

Yes, and didn't imply non-ownership either. We have all these nice smart pointers but per Sutter's advice, we're still also stuck using a raw pointer that tells us nothing. I did not advocate changing the meaning of a raw pointer (terrible idea, I'm sure you'll agree); what I want is something that explicitly states its use case. The whole point of having a name for a smart pointer is to tell us it's use case, like "shared_ptr" and "unique_ptr".

I am aware that the use case for 'non-owning' can be considered 'raw pointer', but that's a luxury way of thinking because there's so much code already written that uses the raw pointer type in different ways (owning/non-owning).

For example, anyone who feels strongly about this are free to use the encapsulation features of the language:

Great idea. Rename "Better" to "WBetter", donate it to the Wt guys and pass me back a reference to my object. Problem solved. (Side note, I don't delete addresses of references)

std::observer_ptr: I like the idea but the concrete proposal seems quite stupid. Poor name and lifetime management methods that do nothing.

Easier just to add a comment...

I don't comment my code. :) Seriously though, I prefer a type rather than comments. The compiler can't enforce my comment.

RE: wt-4 changes - Added by Gunnar Skogsholm 10 months ago

uses the raw pointer type in different ways (owning/non-owning)

No, as I've said, raw ptr doesn't imply either owning or non-owning. The only thing that implies "owning" in C++98 code is the new operator.

Rename "Better" to "WBetter"

They did the right thing. Encapsulation of their interface is your job.

RE: wt-4 changes - Added by Velvet Jones 10 months ago

They did the right thing. Encapsulation of their interface is your job.

Well I guess we finally nailed down this discussion, and it's been useful to me. I personally think that if I am shipping an interface, I should make it bullet-proof and obvious in order to ease adoption (performance & correctness aside). During the initial part of this discussion I decided that the best I can do is encapsulate their interface myself, but nothing stops their interface from bleeding out raw pointers whose use may be misunderstood. So I am of the opinion that it was their job. That said, to quote your original post, "water under the bridge."

And to the Wt developers - no offense meant, I just disagree on a choice you made.

(1-13/13)