Remove some naked new pointers #15

jmigual · 2017-01-24T20:21:37Z

I've replaced some naked new pointers. Now I've used QSharedPointer instead. Also I've added some initializer lists now that we suport VS2015.

jmigual · 2017-01-24T22:45:08Z

Added make_parented in createPaneWidget

daschuer · 2017-01-25T09:58:46Z

src/library/libraryfeature.cpp

+parented_ptr<WTrackTableView> LibraryFeature::createTableWidget(int paneId, 
+            const parented_ptr<QWidget>& parent) {
+    auto pTrackTableView = make_parented<WTrackTableView>(
+            parent.get(), m_pConfig, m_pTrackCollection, true);


I would prefer to use this interface:

parented_ptr<WTrackTableView> LibraryFeature::createTableWidget( int paneId, QWidget* parent)

because there is no requirement that the parent for the new object is parented as well. The lifetime can be also tracked by a unique or shared pointer.

In general I prefer passing plain pointer to functions, if the function does not store the pointer.
In this case the ownership does not matter.

@uklotzde: I might remember you have posted a link to a book about that, but I cannot find it.
Is my assumption correct?

I would primarily use parented_ptr (by value or const-ref) for return values to document the ownership. I'm not sure if this actually matters for parameters?

Possible rule: By accepting a raw pointer the called function promises not to use the pointer after returning. But passing it over to Qt for parenting a new object would violate this rule. So maybe a parented_ptr is more suitable here?

Possible rule: By accepting a raw pointer the called function promises not to use the pointer after returning. But passing it over to Qt for parenting a new object would violate this rule. So maybe a parented_ptr is more suitable here?

Ok, that is one possible point of view. I originally had a different interpretation, now I am unsure.

QObject stores actually the parent pointer, but it does not own it. It is treated like a weak pointer, which is always checked against nullptr before usage.
So if we follow the rule "A raw pointer (a T*) is non-owning" it will applies to this parent pointer, and would allow to use a raw pointer parent parameter. A parented_ptr is also a non-owining pointer, but forces us to parent the passed object before passing. A raw pointer as parameter will allow to use pointers managed by shared and unique_ptrs as well.

Both solutions are good to me in this case. If we like to establish a general rule here, we have to decide if we want to force the parent pointer to have already a parent itselves.
Is this a good idea or not?

Thoughts?

For me it's a good idea that a parent pointer should have a parent too, however this escalates through the LegacySkinParser and for the moment I've stopped there. All the LegacySkinParser should work with the parented_ptr instead of passing raw pointers.

Ok, this means we should stick with the "const parented_ptr& parent" parameter. Right?
Fine, we have a decision. :-)

daschuer · 2017-01-25T10:19:55Z

src/library/features/baseplaylist/baseplaylistfeature.cpp

@@ -680,25 +679,25 @@ void BasePlaylistFeature::slotAnalyzePlaylist() {
 }

 TreeItemModel* BasePlaylistFeature::getChildModel() {
-    return m_childModel;
+    return m_childModel.data();


I think this should return the shared pointer here. If this is an override function, to meet an Qt interface it should be marked as override in the header file and here with a comment.

Mmm it's not a Qt interface but it's our interface. We pass the TreeItemModel to a Qt interface later so returning the shared pointer implies changing the m_childModel from all the library features. Maybe it's worth the work to prevent a break.

It looks like you will inherit a lot of work, continuing this route. For me it is OK to stop here and just add the comments. I think it is OK with the strategy to first replace itching new() calls with the new concept. If we treat raw pointer as non owning pointers the code should become clear.
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#r3-a-raw-pointer-a-t-is-non-owning

daschuer · 2017-01-25T10:23:51Z

src/library/features/baseplaylist/baseplaylistfeature.h

    TrackPointer m_pSelectedTrack;

+    QSharedPointer<TreeItemModel> m_childModel;


Use std:shared_ptr here. We have found out that it is more efficient in Qt 5.
mixxxdj#1132 (comment)

Ok, fine I had my doubts in using QSharedPointer now you've resolved them 😊

daschuer · 2017-01-25T10:27:32Z

src/library/features/playlist/playlistfeature.cpp

@@ -22,6 +22,7 @@ PlaylistFeature::PlaylistFeature(UserSettingsPointer pConfig,
                                 TrackCollection* pTrackCollection)
        : BasePlaylistFeature(pConfig, pLibrary, parent, pTrackCollection) {
    //construct child model
+    m_childModel = QSharedPointer<TreeItemModel>(new TreeItemModel(this));


Why is this shared?

Is this a typo? TreeItemModel has "this" as parent.

It's shared because PlaylistFeature inherits BasePlaylistFeature and m_childModel is shared between the base class and the inherited class. This is because HistoryFeature uses a HistoryTreeModel instead of a TreeItemModel.

And yes this is a typo, I confused it with the HistoryTreeModel class which needs a LibraryFeature as a constructor.

daschuer · 2017-01-25T10:33:27Z

src/library/features/history/historyfeature.cpp

-    delete m_childModel;
-    m_childModel = m_pHistoryTreeModel = new HistoryTreeModel(this, m_pTrackCollection);
+    m_childModel = m_pHistoryTreeModel = QSharedPointer<HistoryTreeModel>(
+            new HistoryTreeModel(this, m_pTrackCollection));


It looks like this is also already parented.

No this is not parented because HistoryTreeModel needs the feature which is holding it to work.

daschuer · 2017-01-25T11:41:38Z

src/library/librarysidebarexpandedmanager.h

@@ -7,7 +7,7 @@ class LibrarySidebarExpandedManager : public LibraryPaneManager
  public:
    LibrarySidebarExpandedManager(Library* pLibrary, QObject* parent = nullptr);

-    void bindPaneWidget(WBaseLibrary* sidebarWidget,
+    void bindPaneWidget(QPointer<WBaseLibrary> sidebarWidget,


QPointer only says: Be null if the references Object is delete. From Qt5 this is exactly the same as a weak reference by QWeakPointer. I think this should not be part of an interface, because it is pointless here.

In this case the LibrarySidebarExpandedManager will store the pointer. It should be guaranteed that the lifetime is controlled by someone else. In case of shared / weak pinter, we would pass a shared pointer and store it as weak pointer.

I think in this case we can use our parented_ptr<> meaning: Give me a QObject that has a parent. Than we can store it as QPointer

It has been added to LibraryPaneManager and LibrarySidebarExpandedManager and also to the library and the legacy skin parser calls.

daschuer · 2017-01-29T11:02:22Z

src/library/features/baseplaylist/baseplaylistfeature.h

    TrackPointer m_pSelectedTrack;

+    std::shared_ptr<TreeItemModel> m_childModel;


This protected m_childModel is a bit scary for me, because it is instantiated in the inherited classes.
Can we move it to the inherited classes and add here an abstract getChildModel instead?

and when it remains a pointer type, it should be called m_pChildMode, .... but is it required to be a pointer.

daschuer · 2017-01-29T11:09:11Z

src/library/features/history/historyfeature.cpp

-    delete m_childModel;
-    m_childModel = m_pHistoryTreeModel = new HistoryTreeModel(this, m_pTrackCollection);
+    m_childModel = m_pHistoryTreeModel = 
+            std::make_shared<HistoryTreeModel>(this, m_pTrackCollection);


Ah yes, m_pHistoryTreeModel and m_childModel could be merged to a non pointer member.
We just need to implement the abstract getChildModel here.

Did you consider to get rid of this shared pointer.
We just tor it in the object and the inherited base object.

jmigual · 2017-02-07T10:46:35Z

To finish this PR we must wait until mixxxdj#1161 is completed or there's something that I can do meanwhile?

timrae · 2017-02-07T19:49:52Z

It's merged now, thanks for the patience

daschuer · 2017-02-07T19:52:59Z

I am just working on merging master to daschuer:jmigual-library-redesign
It conflicts a lot.

timrae · 2017-02-07T19:59:34Z

Now that we've solved the coding style issues at the project level and forked off the 2.1 branch I think we can get this library redesign merged in the next week or so with some good collaboration, what do you guys think?

daschuer · 2017-02-07T22:38:03Z

We have the 2.1 branch now, mainly for testing th CI. I think most of the work currently done in master will be still merged to 2.1.

But that should us not bother here. If we think we are ready we can advertise this for the next release.
One week seems to be tough, because I have also some other task on my list targeted for 2.1.

jmigual · 2017-02-08T08:34:42Z

Yes I will be adding changing all the naked pointers during this week and try to fix as many bugs as possible.

daschuer · 2017-02-08T08:51:35Z

Great, Thank you!

…edesign' Conflicts: src/library/features/crates/cratefeature.cpp

jmigual · 2017-02-09T20:07:08Z

Ok, I've fixed it.

jmigual · 2017-02-09T20:16:38Z

By the way I don't know why but SQLITE3 crashes when creating a collation when using QT5.7.1.0 on Ubuntu 16.04

timrae · 2017-02-10T12:17:33Z

Let me take care of the move constructor issue, we don't want to disperse Joan's time too much

…

On 10 Feb. 2017 21:14, "Daniel Schürmann" ***@***.***> wrote: Strange, gcc is still trying to use the type conversion function. What happens if you comment it out? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#15 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACsA4gYuiN37yIDV6u5hspV3Rd2soiUqks5rbFSsgaJpZM4LsvU0> .

jmigual · 2017-02-10T12:19:39Z

Strange, gcc is still trying to use the type conversion function.
What happens if you comment it out?

src/library/features/analysis/analysisfeature.cpp: In member function ‘virtual parented_ptr<QWidget> AnalysisFeature::createPaneWidget(KeyboardEventFilter*, int, QWidget*)’:
src/library/features/analysis/analysisfeature.cpp:73:12: error: could not convert ‘pTable’ from ‘parented_ptr<WTrackTableView>’ to ‘parented_ptr<QWidget>’
     return pTable;
            ^
src/library/features/analysis/analysisfeature.cpp: In member function ‘virtual parented_ptr<QWidget> AnalysisFeature::createInnerSidebarWidget(KeyboardEventFilter*, QWidget*)’:
src/library/features/analysis/analysisfeature.cpp:96:12: error: could not convert ‘pAnalysisView’ from ‘parented_ptr<DlgAnalysis>’ to ‘parented_ptr<QWidget>’
     return pAnalysisView;
            ^

It seems that the problem is when casting from a different type.

jmigual · 2017-02-10T12:20:44Z

These are the same lines that have been giving us problems before commenting the type conversion function.

timrae · 2017-02-10T14:05:29Z

I can't even get mixxx to start compiling with gcc-4.8 on Ubuntu 16.10... Scons (v 2.5) complains that "A compiler with C++11 support is required." I guess they removed support for 4.8 :-/ gcc-4.9 is working so I'll see if I can reproduce with that.

…

On Fri, Feb 10, 2017 at 9:20 PM, Joan ***@***.***> wrote: These are the same lines that have been giving us problem before commenting the type conversion function. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#15 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACsA4l-ReNFiUyJtPISQ-lcBvyc4f3w2ks5rbFYcgaJpZM4LsvU0> .

daschuer · 2017-02-10T20:13:47Z

There is still this redundant shared pointer member. Can't we get rid of it?

+    m_childModel = m_pHistoryTreeModel = 
+            std::make_shared<HistoryTreeModel>(this, m_pTrackCollection);

jmigual · 2017-02-10T23:01:26Z

There is still this redundant shared pointer member. Can't we get rid of it?

Yes, now it's done 😊

By the way I found that the issue with SQLITE3 and Qt5 is not due to the Ubuntu distributed version of Qt5 but the version distributed with the Qt Maintenance Tool, has anyone tried using it to install Qt5? In Windows it crashes too and I have to compile Qt5 to get it to work.

daschuer · 2017-02-10T23:05:20Z

Thank you. Did you forget to push?

jmigual · 2017-02-10T23:07:02Z

Oh I added the changes in the wrong branch sorry! I'll try to fix it

Conflicts: src/library/features/mixxxlibrary/mixxxlibraryfeature.cpp

jmigual · 2017-02-10T23:09:30Z

Done

daschuer · 2017-02-10T23:24:23Z

Thank you! Sorry for beeing not able to merge this earlier.

timrae · 2017-02-10T23:43:13Z

This doesn't compile due to a typo :-/

src/library/features/mixxxlibrary/mixxxlibraryfeature.cpp:149:22: error: ‘pSidebaar’ was not declared in this scope
     return std::move(pSidebaar);

timrae · 2017-02-10T23:43:49Z

Do you not get the CI check on your fork?

jmigual · 2017-02-10T23:53:27Z

This doesn't compile due to a typo :-/

Oh sorry it's due to a bad merge. Now I've fixed it and checked that it compiles #16.

Do you not get the CI check on your fork?

Yes but it takes about 30 minutes to compile and send me a failed email 😅

timrae · 2017-02-11T01:59:32Z

I got gcc4.8 up and running on my old laptop and was able to confirm that it doesn't like the polymorphic type. If we return parented_ptr<DerivedClass> instead of parented_ptr<QObject> then it works as expected. The code as it is works fine on gcc5 so I think it's just a gcc bug, but I'll check the unique_ptr implementation for some hints on how to fix it for gcc4.8.

timrae · 2017-02-11T02:42:49Z

Actually unique_ptr doesn't work here either... The following example does not compile with gcc 4.8

#include <iostream>
#include "util/parented_ptr.h"
#include "util/memory.h"

using namespace std;

struct Test : public QObject {
    void foo() { std::cout << "foo" << std::endl; }
};

unique_ptr<QObject> foo()
{
  auto p = make_unique<Test>();
  return p;                   // 1
  //return move( p );         // 2
}

timrae · 2017-02-11T03:12:55Z

@daschuer @rryan
How about just making the jump from C++11/gcc-4.8 to C++14/gcc-5.2?
I mean in Mixxx 2.2 obviously

daschuer · 2017-02-11T10:44:01Z

This means we will drop Ubuntu Precise support. This issue here is IMHO not important enough for the switch.

Looking to the issue again, it is probably not bad to have the return std::move(pAnalysisView) in case of a type conversion.

parented_ptr<QWidget> AnalysisFeature::createInnerSidebarWidget(
            KeyboardEventFilter* pKeyboard, QWidget* parent) {
    auto pAnalysisView = make_parented<DlgAnalysis>(parent, this, m_pTrackCollection);
 return std::move(pAnalysisView);
 }

The only issue is that it is a source of CI errors, because without it it fails only on some targets.

daschuer · 2017-02-11T11:35:59Z

I have a solution:

    template <typename U>
    operator parented_ptr<U>() const {
        return *this;
    }

Works, unfortunately it allows also up-casts. So we should make it gcc 4 only.

Alternatively we can define

    operator parented_ptr<QWidget>() const {
        return *this;
    }

this does not work. It compiles, but does not make a change, maybe this is a implicit rule anyway.

    operator parented_ptr<typename std::enable_if<std::is_convertible<U*, T*>::value>() const {
        return *this;
    }

daschuer · 2017-02-11T13:53:09Z

How about this solution:

    template <typename U>
    operator parented_ptr<U>() const {
        static_assert(std::is_convertible<T*, U*>::value, 
                "No implicit conversion from T* to U* found.");
        return *this;
    }

jmigual · 2017-02-11T14:11:30Z

I like this last solution.

rryan · 2017-02-11T17:00:12Z

This means we will drop Ubuntu Precise support. This issue here is IMHO not important enough for the switch.

Precise is already dropped for 2.1 -- did you mean Trusty?

There is already a new LTS release with Xenial. By the time 2.2 comes out it will already have been plenty of time for users to upgrade (it's already been nearly a year).

I agree this is a pretty minor reason to force a switch. I'm more concerned that Trusty has an ancient version of Qt5 (5.2) so continuing support for it will cause an unnecessary burden on development.

daschuer · 2017-02-11T17:29:54Z

Yes, of cause Trusty. We will see, what is the next reason for a drop, the time will come. :-)

Deere 2.1 layout & color fixes

controllers/bulk: Fix BulkController constructor

Faad2 + CMake

…h sync When loading a track that is not yet present in the library (and thus doesn't have any BPM because it hasn't been analyzed yet) while another deck is playing and both decks have sync enabled, a debug assertion is triggered: DEBUG ASSERT: "isValid()" in function double mixxx::Bpm::value() const at src/track/bpm.h:53 Aborted (core dumped) The backtrace looks as follows: #0 0x00007f175c87234c in __pthread_kill_implementation () at /usr/lib/libc.so.6 #1 0x00007f175c8254b8 in raise () at /usr/lib/libc.so.6 #2 0x00007f175c80f534 in abort () at /usr/lib/libc.so.6 #3 0x00007f175cf05ee4 in qt_assert(char const*, char const*, int) () at /usr/lib/libQt5Core.so.5 #4 0x000055deb2e67e1c in mixxx::(anonymous namespace)::handleMessage(QtMsgType, QMessageLogContext const&, QString const&) (type=<optimized out>, context=<optimized out>, input=<optimized out>) at src/util/logging.cpp:355 #5 0x00007f175cf47128 in () at /usr/lib/libQt5Core.so.5 #6 0x00007f175cf3fd8a in () at /usr/lib/libQt5Core.so.5 #7 0x00007f175cf06526 in QMessageLogger::critical(char const*, ...) const () at /usr/lib/libQt5Core.so.5 #8 0x000055deb2e5c720 in mixxx_debug_assert(char const*, char const*, int, char const*) (assertion=assertion@entry=0x55deb39bd0db "isValid()", file=file@entry=0x55deb39bbf30 "src/track/bpm.h", line=line@entry=53, function=function@entry=0x55deb39bbf08 "double mixxx::Bpm::value() const") at gsrc/util/assert.h:9 #9 0x000055deb2ee7e7e in mixxx_debug_assert_return_true(char const*, char const*, int, char const*) (function=0x55deb39bbf08 "double mixxx::Bpm::value() const", line=53, file=0x55deb39bbf30 "src/track/bpm.h", assertion=0x55deb39bd0db "isValid()") at gsrc/util/assert.h:18 #10 mixxx::Bpm::value() const (this=<synthetic pointer>) at src/track/bpm.h:53 #11 mixxx::operator*(mixxx::Bpm, double) (multiple=1, bpm=...) at src/track/bpm.h:160 #12 SyncControl::setLocalBpm(mixxx::Bpm) (this=<optimized out>, localBpm=...) at src/engine/sync/synccontrol.cpp:567 #13 0x000055deb34c7ba3 in EngineBuffer::postProcess(int) (this=0x55deb56eb060, iBufferSize=2048) at src/engine/enginebuffer.cpp:1318 #14 0x000055deb3139023 in EngineMaster::processChannels(int) (this=0x55deb5449440, iBufferSize=<optimized out>) at src/engine/enginemaster.cpp:383 #15 0x000055deb31394f7 in EngineMaster::process(int) (this=0x55deb5449440, iBufferSize=iBufferSize@entry=2048) at src/engine/enginemaster.cpp:410 #16 0x000055deb2f91d0b in SoundManager::onDeviceOutputCallback(long) (this=<optimized out>, iFramesPerBuffer=iFramesPerBuffer@entry=1024) at src/soundio/soundmanager.cpp:596 #17 0x000055deb32dd794 in SoundDevicePortAudio::callbackProcessClkRef(long, float*, float const*, PaStreamCallbackTimeInfo const*, unsigned long) (this=0x55deb553e6b0, framesPerBuffer=1024, out=<optimized out>, in=<optimized out>, timeInfo=<optimized out>, statusFlags=<optimized out>) at src/soundio/sounddeviceportaudio.cpp:965 This happens because `newLocalBpm` is invalid when `localBpm` is invalid. Trying to do sync decks while no tempo information is available does not make sense, so we only synchronize decks if the local BPM is available.

jmigual added 4 commits January 24, 2017 19:52

Post VS2015 change adding initializer lists.

05fb2f1

Remove naked new pointer

2e510d0

Use make_parented for createPaneWidget function

b724ec7

Merged branch feature/useParented into jmigual-library-redesign

673c118

daschuer reviewed Jan 25, 2017

View reviewed changes

jmigual added 3 commits January 26, 2017 00:00

Add shared pointer instead of QShared

222a7d5

Remove some duplicated headers in BasePlaylistFeature

3bf9d76

Add parented_ptr to all functions that require it

fbfe632

It has been added to LibraryPaneManager and LibrarySidebarExpandedManager and also to the library and the legacy skin parser calls.

daschuer mentioned this pull request Jan 26, 2017

Introduce "parented_ptr" wrapper class mixxxdj/mixxx#1142

Merged

daschuer reviewed Jan 29, 2017

View reviewed changes

daschuer mentioned this pull request Jan 31, 2017

Update parented_ptr mixxxdj/mixxx#1161

Merged

jmigual added 3 commits February 9, 2017 18:42

Merge remote-tracking branch 'refs/remotes/daschuer/jmigual-library-r…

cb42197

…edesign' Conflicts: src/library/features/crates/cratefeature.cpp

Add unique_ptr only when it's necessary

36db4b1

Add parent to sidebar expanded

91d2fc2

jmigual added 2 commits February 10, 2017 23:39

Typo

ea8c2e3

Remove unnecessary m_childmodel

ceebf6a

Merge branch 'non-std-move'

f701853

Conflicts: src/library/features/mixxxlibrary/mixxxlibraryfeature.cpp

daschuer merged commit 0125431 into daschuer:jmigual-library-redesign Feb 10, 2017

daschuer pushed a commit that referenced this pull request Dec 11, 2017

Merge pull request #15 from ronso0/deere_2.1_layout-fixes

10768df

Deere 2.1 layout & color fixes

daschuer pushed a commit that referenced this pull request Jun 15, 2020

Merge pull request #15 from Holzhaus/bulk-controller-fix

97d5623

controllers/bulk: Fix BulkController constructor

daschuer pushed a commit that referenced this pull request Jun 15, 2020

Merge pull request #15 from daschuer/faad2

c775f33

Faad2 + CMake

		TrackPointer m_pSelectedTrack;

		QSharedPointer<TreeItemModel> m_childModel;

		TrackPointer m_pSelectedTrack;

		std::shared_ptr<TreeItemModel> m_childModel;

Remove some naked new pointers #15

Remove some naked new pointers #15

Conversation

jmigual commented Jan 24, 2017

jmigual commented Jan 24, 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

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

jmigual commented Feb 7, 2017

timrae commented Feb 7, 2017

daschuer commented Feb 7, 2017

timrae commented Feb 7, 2017

daschuer commented Feb 7, 2017

jmigual commented Feb 8, 2017

daschuer commented Feb 8, 2017

jmigual commented Feb 9, 2017

jmigual commented Feb 9, 2017

timrae commented Feb 10, 2017 via email

jmigual commented Feb 10, 2017

jmigual commented Feb 10, 2017 • edited Loading

timrae commented Feb 10, 2017 via email

daschuer commented Feb 10, 2017

jmigual commented Feb 10, 2017 • edited Loading

daschuer commented Feb 10, 2017

jmigual commented Feb 10, 2017

jmigual commented Feb 10, 2017

daschuer commented Feb 10, 2017

timrae commented Feb 10, 2017

timrae commented Feb 10, 2017

jmigual commented Feb 10, 2017

timrae commented Feb 11, 2017

timrae commented Feb 11, 2017

timrae commented Feb 11, 2017 • edited Loading

daschuer commented Feb 11, 2017

daschuer commented Feb 11, 2017

daschuer commented Feb 11, 2017

jmigual commented Feb 11, 2017

rryan commented Feb 11, 2017

daschuer commented Feb 11, 2017

jmigual commented Feb 10, 2017 •

edited

Loading

jmigual commented Feb 10, 2017 •

edited

Loading

timrae commented Feb 11, 2017 •

edited

Loading