Opened 7 years ago

Closed 6 years ago

#13069 closed Developer Task (Fixed)

Fix QObject related compile warnings in trunk

Reported by: David Hampton <mythtv@…> Owned by: David Hampton
Priority: minor Milestone: 29.0
Component: MythTV - General Version: Master Head
Severity: medium Keywords:
Cc: Ticket locked: no

Description

These fixes originated from compiler warnings generated from adding the -Wextra flag to the build. This pull request only addresses changes related to fixing this error:

In copy constructor ‘blah::blah(const blah&)’:

warning: base class ‘class blah’ should be explicitly initialized in the copy constructor [-Wextra]

Implementing explicit initializers for these objects results in an error instead of a warning:

In copy constructor ‘blah::blah(const blah&)’:

error: ‘QObject::QObject(const QObject&)’ is private within this context : QObject(other) /usr/include/qt5/QtCore/qobject.h: note: declared private here Q_DISABLE_COPY(QObject)

Fix these errors by doing the following:

1) FileSystemInfo? and MetaGrabberScript? don't use any of the features of QObject, so make them standalone classes.

2) Remove public copy constructors from MythSystemLegacy?. This object uses Qt signals, so it has to be based on QObject.

3) Remove the copy constructor from MythCookieJar?. Do this by splitting it into its two components; creating a new jar and copying cookies from one jar to another. This is probably not the ideal solution, but it has the smallest impact on the existing code base. (An better solution would allow cookiejars to be created once, and then copy cookies back and forth. This would eliminate the need to destroy and recreate the jar on each copy.)

4) Clean up the RecStatus? object. This object is essentially an enum with some related functions. It doesn't ever appear to actually be instantiated as an object. This object is based on QObject and therefore the object itself cannot be used with the Qt metatype system. The pointer, however, is automatically registered with the metatype system (as are pointers to all QObject derived classes). Remove the copy constructor and the unnecessary/duplicate metatype declaration/registration code.

5) Clean up all the data/service contract objects that are based on QObject. Like RecStatus?, the objects themselves cannot be used with the Qt MetaType? system, and the pointers are automatically registered. This allows all of the declaration/registration code to be removed from the files. Removing the registration code allows all of the InitializeCustomTypes? functions go away, which allows the service object constructors to be cleaned up.

The CopyListContents? code also needs to be simplified to not convert from pointer to object to reference, because this causes an implicit copy of the object. That means that all the object copy functions need to be updated to use pointers instead of references.

6) Create a test framework for the data contracts. This currently builds five different types of objects, converts them to a QVariant and back, and then compares the results to the original. It should be expanded to all the data objects, and other tests added.

These changes have been tested by working through most of the menu items in mythfrontend, and by working through most of the items in the new web frontend.

Change History (6)

comment:1 Changed 7 years ago by David Hampton <mythtv@…>

comment:2 Changed 6 years ago by David Hampton

Owner: set to David Hampton
Status: newaccepted

comment:3 Changed 6 years ago by David Hampton <mythtv@…>

In 527fca95c3d4c6c98a2041d9cec5e0d023ecf400/mythtv:

FileSystemInfo? and MetaGrabberScript? don't need to be based on QObject.

These classes don't use any of the features of QObject, so removing its
dependency on QObject is the simplest chage to eliminate the warning
message:

warning: base class ‘class QObject’ should be explicitly
initialized in the copy constructor [-Wextra]

Refs #13069

comment:4 Changed 6 years ago by David Hampton <mythtv@…>

In 030ecdecc5603ba8e583a5ea04e7205efd314175/mythtv:

Remove public copy constructors from MythSystemLegacy?.

The MythSystemLegacy? object is based on QObject and therefore
shouldn't have a public copy constructor. Update the code to
properly initialize the parent QObject, and to remove the
unused copy constructor.

General: Instances of QObject (and its subclasses) should be
considered unique items that cannot be copied. Knowing that each
object has only one parent and an arbitrary number of children makes
it easier to understand this. Even if you "clone" the object by
setting all the member variables to the same values, you can't set the
parent and child variables to the same values without screwing up the
one-to-many relationship of parent to children.

See:
http://doc.qt.io/qt-5/qobject.html#Q_DISABLE_COPY
https://www.ics.com/designpatterns/book/qobject.html

Refs #13069

comment:5 Changed 6 years ago by David Hampton <mythtv@…>

In 752a0070cbeed18eccb322c1c103eb51370bc665/mythtv:

Eliminate MythCookieJar? copy constructor.

MythCookieJar? fix attempt #3.

The MythCookieJar? object is based on QObject and therefore shouldn't
have a public copy constructor. Unfortunately its two main purposes
are to load/save cookies from/to a file, and to copy cookies between
multiple cookie jars. This fix simply splits the copy constructor
into its two components; creating a new jar and copying cookies from
one jar to another. It is probably not the ideal solution, but it has
the smallest impact on the existing code base.

General: Instances of QObject (and its subclasses) should be
considered unique items that cannot be copied. Knowing that each
object has only one parent and an arbitrary number of children makes
it easier to understand this. Even if you "clone" the object by
setting all the member variables to the same values, you can't set the
parent and child variables to the same values without screwing up the
one-to-many relationship of parent to children.

See:
http://doc.qt.io/qt-5/qobject.html#Q_DISABLE_COPY
https://www.ics.com/designpatterns/book/qobject.html

Refs #13069

comment:6 Changed 6 years ago by David Hampton

Resolution: Fixed
Status: acceptedclosed

Fixed by commit b3938fab.

Last set of commits for cleaning up QObject related warnings from -Wextra.

1) Cleanup QObject related warnings for RecStatus?. This class is

based on QObject, so only a pointer to it can be registered with the Qt MetaType? system, not the object itself. Also remove the copy constructor.

2) Cleanup QObject related warnings for all the data/service contract

objects. These are all also based on QObjects, so only a pointers to them can be registered with the Qt MetaType? system, not the objects themselves. Also cleanup unnecessary registration code.

3) Start on a test framework for the data contracts. This needs to be

extended to support all the contracts.

Fixes #13069.

Note: See TracTickets for help on using tickets.