Fix inheritance of LocationDelegate #2300
Conversation
|
|
||
| class TableItemDelegate : public QStyledItemDelegate { | ||
| Q_OBJECT | ||
| public: | ||
| explicit TableItemDelegate(QTableView* pTableView); | ||
| virtual ~TableItemDelegate(); | ||
| ~TableItemDelegate() override = default; |
There was a problem hiding this comment.
This is redundant on one hand, but I remember that there was an issue with a clang compiler warning. Is this still the case? Do we have a number of these compiler warnings elsewhere?
If yes, we should think about a mass fix and drop a note here: https://www.mixxx.org/wiki/doku.php/coding_guidelines
There was a problem hiding this comment.
This is not redundant! It verifies that the destructor of the base class is virtual.
There was a problem hiding this comment.
Yes right, but is this important here?
But nevermind, it is a question if code style. We just need to state somewhere if we want that line or not.
Does this issue a compiler warning, btw?
There was a problem hiding this comment.
I don't know and don't really care. If this pattern is not desired then simply remove all lines that match the regex "~.*override = default;" in a separate commit.
There was a problem hiding this comment.
I have read a bit what others think. While I originally was the opinion that it is unnecessary noise, this post convinced me that it is actually sensible isocpp/CppCoreGuidelines#721 (comment)
I have added a sentence to our style guides:
This applies also for default destructors even if it looks noisy.
somehow it does, at least the OS style can be overwritten. before: this PR: |
|
welcome! thx to @uklotzde for the quick fix. Manual testing would be a no-brainer if there where CI build artifacts for linux.. |
|
But I have no idea how to correctly propagate the highlighted text color to the delegate. This should be done in the common base class. |
...including some code cleanup in the base class TableItemDelegate.