Introduce "parented_ptr" wrapper class #1142

timrae · 2017-01-22T00:45:09Z

Proposed change to the coding guidelines:

No naked new statements.

If a pointer is to be managed by the QT object tree, and has a known parent at the time of creation, use make_parented<T>
If a pointer is not to be managed by the QT object tree, use std::make_unique<T> to create the object, or std::make_shared<T> if shared ownership is really necessary.
If a pointer that will eventually be managed by the object tree needs to be created without a parent, create the object p with std::make_unique<T>, then later create a new variable using parented_ptr(p.release()) once the underlying object gets a parent.
If a raw pointer is passed into or out of a function, it should be assumed that the pointer is being "borrowed". Keeping a pointer (or reference) to the underlying object violates this assumption and should be avoided. Instead use std::move() with a unique_ptr if ownership is to be transferred, or if shared ownership is really necessary then use shared_ptr.
As per the previous rule, in general we can say that a class should not have raw pointers as member variables. Always wrap pointer member variables in parented_ptr<T>, unique_ptr<T>, or shared_ptr<T>.
Any exceptions to these guidelines must be clearly documented inline in the code

Reasons for the change
It's currently extremely difficult to review new and existing mixxx code for memory leaks and dangling pointers because the code base is scattered with naked new calls, and ownership of pointers is almost never clear. Modern C++ discourages using new and delete, so we should localize usage of these to a small number of places where the syntax makes ownership clear.

timrae · 2017-01-22T00:59:59Z

If everyone is happy with the change, then I'll start going through the code base replacing naked new statements with make_QPointer, make_unique, and make_shared

rryan · 2017-01-22T04:28:57Z

If everyone is happy with the change, then I'll start going through the code base replacing naked new statements with make_QPointer, make_unique, and make_shared

BTW I've already done a bit of refactoring in #941 to wrap the core Mixxx services with shared pointers. I plan to merge it as soon as the 2.1 branch is cut (we decided it did not make sense to potentially destabilize 2.1).

In general, we tend to avoid mass refactorings and instead clean up code as we touch it -- so I'd prefer it if you not go through and do some mass updates to get rid of new/delete. This results in fewer mistakes because it's easier to review piecemeal.

timrae · 2017-01-22T05:03:57Z

@rryan

BTW I've already done a bit of refactoring in #941 to wrap the core Mixxx services with shared pointers.

You'll use std::shared_ptr to be consistent with #1132 though, right?

I plan to merge it as soon as the 2.1 branch is cut (we decided it did not make sense to potentially destabilize 2.1).

...and when might that be? :-p Or to put it more constructively, what work remains before you can do that? Why not just cut the branch now, and cherry-pick in the commits from the PRs that are currently under review once they are merged? I.e. make an exception to the "feature freeze" rule for a subset of the currently open PRs.

I'd prefer it if you not go through and do some mass updates to get rid of new/delete.

OK, I agree that it makes sense to make the changes gradually... How about if I localize the changes to the library related code? In order to make progress with #1117 I feel it's necessary to go through and review the ownership/lifetime of all the pointers, and it makes the most sense for me to clarify those issues by going through and enforcing the rules above.

timrae · 2017-01-22T05:08:25Z

Also, do you agree with the guidelines themselves, and does 11f131e look OK?

rryan · 2017-01-22T05:37:34Z

Good idea -- this will help us document object-tree ownership in the code, which will help readability.

QPointer is used for other purposes than indicating lifetime-via-object-tree (for example, if a non-parent class wants to keep a pointer to a QObject but have it automatically null when that object dies). So it's not a sure indicator of ownership to a reader of the code (e.g. if they see it as a class member variable). I'd like to see a custom type for this that has "parent" in the name or something.
QPointer comes with a hidden cost -- it locks a global mutex on creation and destruction because adds a guard to its pointee's QMetaObject, and it looks up / inserts the pointee in a QMultiHash so it can malloc while holding that mutex (potentially blocking any thread that simultaneously runs code that adds a guard to some other object).

I think the main benefit here is to have the code document the ownership of the objects, so I think we'll get the same benefit with something lighter-weight.

What do you think about using a unique_ptr with a do-nothing deleter. Something like:

template <typename T>
struct DoNothingDeleter {
    void operator()(T*) {}
};

template <typename T>
class parented_ptr : std::unique_ptr<T, DoNothingDeleter<T>> {
  public:
    parented_ptr(T* t) : std::unique_ptr<T, DoNothingDeleter<T>>(t) {
        DEBUG_ASSERT(t->parent() != nullptr);
    }
    ~parented_ptr() = default;
};

or maybe instead of hijacking unique_ptr, a simple wrapper around the pointer with an operator* and operator-> would do.

We could call your make_QPointer helper make_parented (which I think more clearly indicates Qt parent ownership).

rryan · 2017-01-22T05:43:02Z

I plan to merge it as soon as the 2.1 branch is cut (we decided it did not make sense to potentially destabilize 2.1).
...and when might that be? :-p

It's been Real Soon Now for a while :) blocked on our build server. As of this week, working builds are back. I'd like to branch ~immediately.

timrae · 2017-01-22T06:25:19Z

What do you think about using a unique_ptr with a do-nothing deleter?

Awesome idea!

rryan · 2017-01-22T07:35:32Z

Also, do you agree with the guidelines themselves,

Yep -- they sound good to me.

One thing:

If a pointer that will eventually be managed by the object tree needs to be created without a parent, create the object p with std::make_unique, then later create a new variable using QPointer(p.get()) once the underlying object gets a parent.

Did you mean p.release()?

timrae · 2017-01-22T08:07:00Z

@rryan
Actually, unique_ptr has no copy constructor, so it's probably not suitable here...

daschuer · 2017-01-22T08:36:42Z

If a pointer that will eventually be managed by the object tree needs to be created without a parent, create the object p with std::make_unique, then later create a new variable using QPointer(p.get()) once the underlying object gets a parent.

This rule does not work well, because p.get() does not moves the ownership. I think we should not use a unique_pointer where moving ownership by copy is required, because of an external API.

I think we are talking basically about these cases
new QObject(pParent); overloads. here we can use something as

and this case is
void QLayout::addWidget(new QWidget()) overloads.

and this:

QWidget* pWidget = widgetFactory(); 
void QLayout::addWidget(pWidget)

for the first case we can do without overhead:

template<class T, class... Args>
inline void make_as_qobject_child(Args&&... args) {
    new T(std::forward<Args>(args)...);
    DEBUG_ASSERT(pObject->parent() != nullptr);
    return pObject;
}

for the late case we need something like:

template<class T, class... Args>
inline later_qobject_child_ptr make_later_qobject_child(Args&&... args) {
    return = later_qobject_child_ptr(new T(std::forward<Args>(args)...));
}

and
(pseudo code)

class later_qobject_child_ptr {
   public:
       later_qobject_child_ptr(QObject* p) {
           DEBUG_ASSERT(p->parent() == nullptr); // if this fails, use make_as_qobject_child
       }

      later_qobject_child_ptr(const later_qobject_child_ptr& other) = delete;
      later_qobject_child_ptr(later_qobject_child_ptr&& other) {
             m_pointer = other;
              other.release(); 
      }

      ~later_qobject_child_ptr() {
           if (p != nullptr && p->parent() == nullptr) {
                delete p;
           }
      }

      release() {
           m_pointer = nullptr;   
      }

   private: 
      QObject* m_pointer;
}

timrae · 2017-01-22T09:12:38Z

for the first case we can do without overhead

Of course we can, but I don't think we should, as it breaks the 5th rule. Instead we should encapsulate the raw pointer in the new parented_ptr class and try to minimize the overhead.

This rule does not work well, because p.get() does not moves the ownership.

I edited the post sometime before you posted that after seeing @rryan's comment (that it should be p.release() instead of p.get()). release() does signal a change of ownership, so I believe we should reuse unique_ptr instead of "reinventing the wheel" with another class.

rryan · 2017-01-22T09:14:13Z

This rule does not work well, because p.get() does not moves the ownership.

I think @timrae meant to write p.release() there.

I think it's fine to say that until the object has a parent, it should be in a unique_ptr in case the scope is unexpectedly terminated (e.g. someone sticks a 'return' statement somewhere between the point the object is created and receives a parent). the unique_ptr will prevent a leak in that case.

~later_qobject_child_ptr() {
if (p != nullptr && p->parent() == nullptr) {

It's not safe to dereference p at any point after the constructor, since you don't know whether p has been deleted by its parent.

timrae · 2017-01-22T12:54:33Z

src/library/cratefeature.cpp

                           UserSettingsPointer pConfig)
-        : LibraryFeature(pConfig),
+        : LibraryFeature(pConfig, pLibrary),


... and we already find our first bug ;)

timrae · 2017-01-22T13:04:20Z

@rryan @daschuer
Please check 0d5fd8f

timrae · 2017-01-22T14:44:06Z

@rryan
I've removed the changes (that were previously in 55d4098) that would conflict with #941 in order to focus in this PR on the parented_ptr change. I added some TODO comments about changing to shared_ptr.

daschuer · 2017-01-22T15:10:26Z

src/util/parented_ptr.h

+        assert(t->parent() != nullptr);
+    }
+
+    parented_ptr() : m_pPtr(nullptr) {}


Can't this be removed?

Ah, clear now, ditch my comment.

daschuer · 2017-01-22T15:11:08Z

src/util/parented_ptr.h

+    }
+
+    operator bool() const {
+        return m_pPtr != nullptr;


Can this ever be false?

Yes, it can. This should be named isNull()

The snake case class name suggests some affinity to the standard C++ types which use operator bool() for this purpose.

daschuer · 2017-01-22T15:13:30Z

src/util/parented_ptr.h

+     */
+    template <typename U>
+    parented_ptr(const parented_ptr<U>& p, typename std::enable_if<std::is_convertible<U*, T*>::value, void>::type * = 0) 
+        : m_pPtr(p.get()) {}


This should be placed above since it is a constructor.
The assert parent not null is missing

daschuer · 2017-01-22T15:22:19Z

src/util/parented_ptr.h

+
+    bool operator!= (const parented_ptr& other) const {
+        return m_pPtr != other.m_pPtr;
+    }


Compare operators with the raw pointer can also be handy.

daschuer · 2017-01-22T15:27:10Z

src/util/parented_ptr.h

+    ~parented_ptr() = default;
+
+    T* get() const {
+        return m_pPtr;


In case of shared and uique pointer get() indicates a pointer usage without passing ownership.
If we use get() here as well, it looks similar, but the whole new pointer does not have the ownership, so I think we can remove get and add a cast operator instead. This way we can get rid of the get cat() calls at the user side.

The implicit cast operator hides unintended conversions to the raw pointer type.

If we use get() here as well, it looks similar, but the whole new pointer does not have the ownership

It kind of does have ownership (by proxy) to the object tree. We could even add a release() function that removes the parent and returns the raw pointer.

Also, it's highly preferable to keep the interface consistent with the other std pointer wrappers to simplify switching between them. I imagine it may be common to switch from shared to parented, and it will be unnecessarily annoying if you have to go and change all the .get() calls to something else.

OK, good point.

daschuer · 2017-01-22T15:34:55Z

What happens now with this case:
void QLayout::addWidget(new QWidget()) overloads.
I think we still miss a solution for it.

Did you consider my psoido code? It may look like
void QLayout::addWidget(make_future_parented<QWidget>())

the pointer can be called future_parented_ptr() or might_parented_ptr()
We can implement it on our own, or we can take a std::unique_ptr with a custom deleter that check for parent() == nullptr.

timrae · 2017-01-22T23:01:55Z

I force pushed a new commit with the changes requested. I also edited my previous comment (just woke up, sorry).

@daschuer
Can you find any existing examples in the code where unique_ptr would be undesirable or akward?

So I could imagine some nice use cases, for factory functions that not requires to take a parent pointer as argument.

Again, here I think unique_ptr is a natural solution.

daschuer · 2017-01-23T00:31:57Z

OK, we can use unique_ptr::release() here. It just has the problem, that it has not the safety against failing to become a parent. A own unique_ptr pointer type that has a custom deleter would document at the make position that this pointer is intended to be owned by the object tree.

rryan · 2017-01-23T00:47:12Z

A own unique_ptr pointer type that has a custom deleter would document at the make position that this pointer is intended to be owned by the object tree.

We could do something like this to handle the variety of different ways an object can gain a parent:

std::parent_checking_unique_ptr foo = make_parent_checking_unique<QMenu>();
// ...
foo.check_parented([this] (QMenu* pMenu) { 
  addMenu(pMenu);
});
// ^ check parented invokes the lambda with the result of release() then debug-asserts the parent is populated.

Or it could be standalone method that you std::move a unique_ptr into:

template <typename T>
void check_parented(std::unique_ptr<T> p, lambda) {
  T* rp = p.release();
  lambda(rp);
  DEBUG_ASSERT(rp->parent() != nullptr);
}

It's probably safe to assume that the object is not deleted by its parent immediately after the end of the lambda. While if you had a custom deleter, you have no way of knowing that by the end of the scope the custom unique_ptr lives in that the object was not already deleted by its parent -- so it's never safe to e.g. check whether something is parented in the deleter.

IMO this is well into "perfect is the enemy of the good" territory here. I like parented_ptr / make_parented as they are now, and the guidelines sound good to me. We don't need to be automatons -- we can always make an exception if the situation warrants.

daschuer · 2017-01-23T00:52:29Z

OK, then it seams the best to me to switch to this solution if the future parent is already known

auto pFileMenu = make_parented<QMenu>(this);
// ...
addMenu(pFileMenu.get());

rryan · 2017-01-23T00:59:59Z

OK, the it seams the best to me to switch to this solution if the future parent is already known

auto pFileMenu = make_parented(this);
// ...
addMenu(pFileMenu.get());

This is worse than using unique_ptr though, since it leaks the QMenu if we return early before hitting addMenu for some reason.

daschuer · 2017-01-23T01:09:31Z

No. You have missed that pFileMenu already is a parent form "this". We have an assertion for that.

rryan · 2017-01-23T01:18:16Z

No. You have missed that pFileMenu already is a parent form "this". We have an assertion for that.

Oh sorry -- I misread. I don't know if that's always desirable -- maybe it's fine for menus, but for widgets, the moment you give it a parent the widget becomes visible if its parent is visible. This can expose an unstyled / incomplete widget and result in wasted draw events as we add more children / set the styling, etc. In this case, using a unique_ptr while you're setting things up seems reasonable to me.

timrae · 2017-01-23T02:13:59Z

It just has the problem, that it has not the safety against failing to become a parent.

I don't think I understand the problem...

auto pFileMenu = std::make_unique<QMenu>();
// ...
addMenu(pFileMenu.release());

Under what circumstances would addMenu() fail to parent the menu item? In terms of safety it isn't any different from the current way... I.e. the following has the exact same risk (or lack thereof) of failed parenting, right?

auto pFileMenu = new QMenu();
// ...
addMenu(pFileMenu);

daschuer · 2017-01-23T08:13:16Z

Under what circumstances would addMenu() fail to parent the menu item?

If the parent object lives in a different thread. A new QObject lives in the creating thread. If you try to parent it to an object from a different thread, it is not adopted as a child and a warning message is issued.

In the
auto pFileMenu = make_parented<QMenu>(this);
case, we haven now a DEBUG_ASSERT guarding against this situation.

In the
auto pFileMenu = std::make_unique<QMenu>(...);
case we receive the warning and the desired code will probably not work.

So if we use the first, it provides additional safety, but the later is no regression compared to the current situation. So all in all the situation is improved and a nice addition to Mixxx.

@rryan
In the whole LegacySkinParser code, we parent the widgets in the constructor. Does it really has a performance penalty?

daschuer · 2017-01-23T08:20:30Z

src/util/parented_ptr.h

+    }
+
+  private:
+    T* m_pPtr;


Can this be m_pObject?

rryan · 2017-01-23T08:22:51Z

In the whole LegacySkinParser code, we parent the widgets in the constructor. Does it really has a performance penalty?

Hm, I thought we constructed the skin with no connection to the MixxxMainWindow until we called setCentralWidget
https://github.com/mixxxdj/mixxx/blob/master/src/mixxx.cpp#L441
but you're right -- we do create the whole skin parented to a visible widget

daschuer · 2017-01-23T08:35:51Z

@timrae:
Could you extract a PR that only introduces the new pointer?
The other changes will likely conflict with the library redesign branch.
I would prefer to introduce the changes there, and save us here from the extra review work in dying code and regression risk in upcoming 2.1.

timrae · 2017-01-23T08:51:15Z

Done (i.e. I updated this PR via force push).
For the record, the commit using parented_ptr in the Library class was 26afb82. I'll start implementing that in the new branch.

daschuer · 2017-01-23T10:32:35Z

LGTM! Thank you. @rryan merge?

rryan · 2017-01-23T17:25:15Z

LGTM! Thank you. @rryan merge?

SGTM

daschuer · 2017-01-23T23:55:44Z

One issue comes into my mind, where we have to be careful:
parented_ptr does unlike unique_ptr and shared_ptr guarantee that the pointer is valid.
::bool() == true, is not an indicator that the pointer is valid.

This means we must not use parented_ptr as a member variable in objects which can outlive the parented object. In this cases we need to use a QPointer.

Something like that should be part of the rules.

timrae · 2017-01-24T00:26:46Z

::bool() == true is not an indicator that the pointer is valid.

You're right, I think that bool() function is not meaningful since it doesn't do what most people will assume it does.

timrae · 2017-01-25T22:48:21Z

@daschuer
In this case, how about we introduce the rule that a "raw" parented_ptr (i.e. without a QPointer wrapper) should only be used as a member variable inside a class where that same class is the parent of the member?

I can't think of any way to get the compiler to enforce this, but we can at least get rid of the bool() operator, and introduce move only semantics (i.e. delete the copy constructor) to improve safety. We can then add another class which automatically wraps the object in a QPointer and adds back the bool() operator and copy constructor.

What do you think? I can't think of a good name for the new class.... Maybe parented_qptr or parented_shared_ptr to indicate that it can be freely passed around like a shared_ptr...?

daschuer · 2017-01-26T00:53:11Z

We have a related discussion in daschuer#15

It looks like our parented_ptr is a bit itching compared to the concepts around it.
I am trying to summarize:

We have the iso gpp guidlines:

A raw pointer (a T*) is non-owning:
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#r3-a-raw-pointer-a-t-is-non-owning
And the owner<T*> p hack in the other way around.
than we have std::unique_ptr and std::shared_ptr which are owning
and QSharedPointer as owning type too
QPointer is the same as QWeakPointer, both non-owning, but guarding against external deletion.
our parented_ptr is a non-owning type, which forces to have a parent with zero overhead, but it does not have the weak pointer feature to be null if the object is gone.

If we have some day all our code refactored to "A raw pointer (a T*) is non-owning", our "parented_ptr" is somehow obsolete, since it dosn't matter for the user code how the lifetime of the pointer is guarded.

The owning issue is a fight gains memory leaks, which are not the worst in the context of the library redesign which is instantiated only once or twice. Far more important in our case are possible crashers to invalid or null pointers.

IMHO this leads to the following rule:

use "parented_ptr" to enforce that the Qobject has a parent
use "QPointer" if the Owner lifetime can be shorter than the user lifetime
use parented_ptr<QPointer> (or a new fancy type) if both applies.

timrae · 2017-01-26T03:42:05Z

use "QPointer" if the Owner lifetime can be shorter than the user lifetime

use parented_ptr<QPointer> (or a new fancy type) if both applies.

Allowing raw QPointer undermines the whole system in my opinion. If the object has no parent then use unique_ptr. If it is 100% obvious that there are no lifetime issues then use parented_ptr. Otherwise use parented_ptr<QPointer>

daschuer · 2017-01-26T08:35:50Z

we have to look at this case

class foo {
  public: 
    addObject1(QObject* p1) {
        m_pObject1 = p1;
    }
   
    addObject1(QObject* p2) {
        m_pObject1 = p1;
    }

    addObject3(parented_ptr<QObject> p3) {
        m_pObject3 = p3;
    }

  private:
    QWeakPointer<QObject> m_pObject1 
    QPointer<QObject> m_pObject2 
    parented_ptr<QPointer<QObject> > m_pObject3 
}

In Qt 5 m_pObject1 and m_pObject2 are the same.

addObject1 and addObject2 allow to take any non-owning pointer:

str::shared_pointer<QObject> p1; 
str::weak_pinter<QObject> p2;
str::unique_ptr<QObject> p3;
QPointer<QObject> p4;
QObject* p5;
parented_ptr<QObject> p6; 

foo.addObject1(p1.get());
foo.addObject1(p2.get());
foo.addObject1(p3.get());
foo.addObject1(p4.data());
foo.addObject1(p5));
foo.addObject1(p6));

while addObject3 allows only parented pointers:

QPointer<QObject> p4;
QObject* p5;
parented_ptr<QObject> p6; 

foo.addObject3(make_parented(p4));
foo.addObject3(make_parented(QPointer(p5))));
foo.addObject3(p6));

So it does make sense to store non parented_ptr in an object.
It is always the case when the object has now requirement how an external ownership is guaranteed.

And yes it undermines the some of our original ideas. But it should not be a problem to tweak our rules a bit for it.

timrae · 2017-01-26T08:50:47Z

Can you please give an actual example from the code base?

…

On 26 Jan. 2017 17:35, "Daniel Schürmann" ***@***.***> wrote: we have to look at this case class foo { public: addObject1(QObject* p1) { m_pObject1 = p1; } addObject1(QObject* p2) { m_pObject1 = p1; } addObject3(parented_ptr<QObject> p3) { m_pObject3 = p3; } private: QWeakPointer<QObject> m_pObject1 QPointer<QObject> m_pObject2 parented_ptr<QPointer<QObject> > m_pObject3 } In Qt 5 m_pObject1 and m_pObject2 are the same. addObject1 and addObject2 allow to take any non-owning pointer: str::shared_pointer<QObject> p1; str::weak_pinter<QObject> p2; str::unique_ptr<QObject> p3; QPointer<QObject> p4; QObject* p5; parented_ptr<QObject> p6; foo.addObject1(p1.get()); foo.addObject1(p2.get()); foo.addObject1(p3.get()); foo.addObject1(p4.data()); foo.addObject1(p5)); foo.addObject1(p6)); while addObject3 allows only parented pointers: QPointer<QObject> p4; QObject* p5; parented_ptr<QObject> p6; foo.addObject3(make_parented(p4)); foo.addObject3(make_parented(QPointer(p5)))); foo.addObject3(p6)); So it does make sense to store non parented_ptr in an object. It is always the case when the object has now requirement how an external ownership is guaranteed. And yes it undermines the some of our original ideas. But it should not be a problem to tweak our rules a bit for it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1142 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACsA4lTz2O3deMuugS3ukeP3sd7AOBJ5ks5rWFrqgaJpZM4LqO0Q> .

daschuer · 2017-01-26T10:27:47Z

I do not know a place in our code where a function is used with different pointer types.
In real live you normally know caller and callee. This is here more a question of good interface design.

If I have a call do(parented_ptr<QObject> p) it tells the caller, I need to access an object with a parent, I will not take the ownership.
This is true in cases like discussed in daschuer#15 (comment) where p is used as parent when creating a new QObject.

On the other hand If I have a call do(QObject* p) it tells a caller: I need to access the object which should be valid during the call. There are no additional assumptions what happens with the object after the call, or how the lifetime is controlled.

I like to be able to use the later version as well in Mixxx.

timrae · 2017-01-29T11:46:44Z

If I have a call do(parented_ptr p) it tells the caller, I need to access an object with a parent, I will not take the ownership.

This is an anti-pattern and would not compile in my proposed modifications due to deleted copy constructor... If a function is not to take ownership, the "correct" way is to do(QObject* p).

On the other hand If I have a call do(QObject* p) it tells a caller: I need to access the object which should be valid during the call. There are no additional assumptions what happens with the object after the call, or how the lifetime is controlled.

I guess you meant do(QPointer<QObject> p)? Because what you wrote is the exact opposite of isocpp guidelines. Assuming it's just a typo, it still doesn't explain the need to pass in a pointer with unspecified ownership. IMO it will almost always be better to have a data type with both ownership and validity guarantees like parented_ptr<QPointer<QObject>>

Anyway, I don't think it's useful to discuss "toy problems" like this without real world concrete examples... I'll submit a PR for my proposed modifications.

timrae · 2017-01-29T14:33:45Z

Let's continue to discuss in #1161

timrae force-pushed the qpointer branch 2 times, most recently from 767fcf3 to 3693277 Compare January 22, 2017 04:19

timrae force-pushed the qpointer branch 2 times, most recently from 0b06a91 to 4586ef7 Compare January 22, 2017 12:51

timrae commented Jan 22, 2017

View reviewed changes

timrae force-pushed the qpointer branch from 4586ef7 to 55d4098 Compare January 22, 2017 13:26

timrae changed the title ~~Introduce make_QPointer function~~ Introduce "parented_ptr" wrapper class Jan 22, 2017

timrae mentioned this pull request Jan 22, 2017

Library Layout Redesign Single pane #1117

Closed

11 tasks

timrae force-pushed the qpointer branch 2 times, most recently from c1ba9dd to e885005 Compare January 22, 2017 14:34

daschuer reviewed Jan 22, 2017

View reviewed changes

daschuer reviewed Jan 23, 2017

View reviewed changes

src/util/parented_ptr.h

}

private:

T* m_pPtr;

Copy link

Member

daschuer Jan 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be m_pObject?

Introduce parented_ptr<T> class which is owned by the QT object tree

5df61f1

timrae force-pushed the qpointer branch from 26afb82 to 5df61f1 Compare January 23, 2017 08:46

rryan merged commit 6b2fd72 into mixxxdj:master Jan 23, 2017

timrae deleted the qpointer branch January 24, 2017 00:01

timrae mentioned this pull request Jan 29, 2017

Update parented_ptr #1161

Merged

Introduce "parented_ptr" wrapper class #1142

Introduce "parented_ptr" wrapper class #1142

Conversation

timrae commented Jan 22, 2017 • edited Loading

timrae commented Jan 22, 2017 • edited Loading

rryan commented Jan 22, 2017 • edited Loading

timrae commented Jan 22, 2017

timrae commented Jan 22, 2017

rryan commented Jan 22, 2017

rryan commented Jan 22, 2017

timrae commented Jan 22, 2017

rryan commented Jan 22, 2017

timrae commented Jan 22, 2017

daschuer commented Jan 22, 2017 • edited Loading

timrae commented Jan 22, 2017 • edited Loading

rryan commented Jan 22, 2017

Choose a reason for hiding this comment

timrae commented Jan 22, 2017

timrae commented Jan 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Jan 22, 2017

timrae commented Jan 22, 2017 • edited Loading

daschuer commented Jan 23, 2017

rryan commented Jan 23, 2017 • edited Loading

daschuer commented Jan 23, 2017 • edited Loading

rryan commented Jan 23, 2017

daschuer commented Jan 23, 2017

rryan commented Jan 23, 2017 • edited Loading

timrae commented Jan 23, 2017 • edited Loading

daschuer commented Jan 23, 2017

Choose a reason for hiding this comment

rryan commented Jan 23, 2017

daschuer commented Jan 23, 2017

timrae commented Jan 23, 2017 • edited Loading

daschuer commented Jan 23, 2017

rryan commented Jan 23, 2017

daschuer commented Jan 23, 2017

timrae commented Jan 24, 2017

timrae commented Jan 25, 2017 • edited Loading

daschuer commented Jan 26, 2017

timrae commented Jan 26, 2017 • edited Loading

daschuer commented Jan 26, 2017

timrae commented Jan 26, 2017 via email

daschuer commented Jan 26, 2017

timrae commented Jan 29, 2017 • edited Loading

timrae commented Jan 29, 2017

timrae commented Jan 22, 2017 •

edited

Loading

timrae commented Jan 22, 2017 •

edited

Loading

rryan commented Jan 22, 2017 •

edited

Loading

daschuer commented Jan 22, 2017 •

edited

Loading

timrae commented Jan 22, 2017 •

edited

Loading

timrae commented Jan 22, 2017 •

edited

Loading

rryan commented Jan 23, 2017 •

edited

Loading

daschuer commented Jan 23, 2017 •

edited

Loading

rryan commented Jan 23, 2017 •

edited

Loading

timrae commented Jan 23, 2017 •

edited

Loading

timrae commented Jan 23, 2017 •

edited

Loading

timrae commented Jan 25, 2017 •

edited

Loading

timrae commented Jan 26, 2017 •

edited

Loading

timrae commented Jan 29, 2017 •

edited

Loading