Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some coding issues #18

Merged
merged 25 commits into from
Mar 1, 2017
Merged

Fix some coding issues #18

merged 25 commits into from
Mar 1, 2017

Conversation

jmigual
Copy link

@jmigual jmigual commented Feb 24, 2017

Fix some (not all) coding issues pointed by @timrae. I've fixed the parented_ptr issue too.

@jmigual
Copy link
Author

jmigual commented Feb 24, 2017

Oh I found an issue related to the parented_ptr class, some classes have the parent() member private and it gives a compilation error. In this case we should force a u to QObject casting and then parented_ptr should be only used with Qt Objects.

See:
https://travis-ci.org/jmigual/mixxx/jobs/205123907

@timrae
Copy link

timrae commented Feb 24, 2017 via email

@daschuer
Copy link
Owner

Actually we can probably remove most of the template code in this case, right?

If we remove the template nature of parented_ptr, we loose the type safety, having an suspicious up cast on every dereferencing access.

I think it is more save to check if the pointer actually points to a QObject inherited class.
std::is_base_of might be uses to verify it.

I do not understand why parent() is private for some classes.
In case of SavedQueriesTableModel it should be public, need to investigate this more.

@jmigual
Copy link
Author

jmigual commented Feb 24, 2017

I've tried doing the casting to QObject but this prevents us from using parented_ptr for TreeItem since it does not inherit QObject so checking for QObject inheritance through std::is_base_of might be a good idea.

@timrae
Copy link

timrae commented Feb 25, 2017

@jmigual
Which classes have private parent() functions and why?

@jmigual
Copy link
Author

jmigual commented Feb 25, 2017

Which classes have private parent() functions and why?

QAbstractTableModel has the QModelIndex parent(const QModelIndex &child) const; member and it seems that the compiler gets confused, it's really strange.

Now I've tried compiling with Qt5.7 and Qt5.5 and it does not give a compilation error. Before I was compiling with Qt4.8.


By the way I found another issue, if you construct a nullptr parented_ptr it gives a segmentation fault related to this line:

explicit parented_ptr(T* t) : m_pObject(t) {
        DEBUG_ASSERT(t->parent() != nullptr);
}

We should check if t == nullptr before doing the assert.

@jmigual
Copy link
Author

jmigual commented Feb 27, 2017

Ok, it's finally fixed the parent() issue with Qt4.8. So this should be ready for merge and I'll add more code fixes later.

I had to modify the parented_ptr class by adding the nullptr comparison with parented_ptr and adding nullptr checking before doing the assert.

@daschuer
Copy link
Owner

daschuer commented Mar 1, 2017

LGTM! Thank you.

@daschuer daschuer merged commit b7b3344 into daschuer:jmigual-library-redesign Mar 1, 2017
daschuer pushed a commit that referenced this pull request Apr 15, 2018
…ehavior

use isAdoptMetaknobValueEnabled and "[Effects]", "AdoptMetaknobValue"
daschuer pushed a commit that referenced this pull request Aug 29, 2021
CMake: fix version detection for TagLib
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants