markg requested changes to this revision.
markg added a reviewer: markg.
markg added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> units.cpp:61
> SharedAppFilter *Units::s_sharedAppFilter = nullptr;
> +Units *Units::s_self = nullptr;
>
Remove this line if you go for the comment i made below.
> units.cpp:98-104
> +Units *Units::self()
> +{
> + if (!s_self) {
> + s_self = new Units();
> + }
> + return s_self;
> }
I think you need to do more, or rather less.
In C++11 making a singleton is really easy! All you need is:
Units &Units::self()
{
static Units units;
return units;
}
And done. It gives you a thread safe singleton (which your current version
isn't).
I also think the current compiler requirements for plasma and frameworks allow
you to use the code I've just given.. So.. Use it :)
Note: i changed the return value to a reference.
Also, why use "self" as getter for a singleton? I think "instance" or
"getInstance" is the most used name for this, but i might be wrong here.
> units.h:122
>
> + static Units *self();
> +
Add documentation please.
> units.h:185-186
> private:
> + Units(QObject *parent = 0);
> + Q_DISABLE_COPY(Units)
> +
If this were pre C++11, you'd be done :)
But we have move semantics now. You need to disable moving as well!
Units(Units const&) = delete; // Copy construct
Units(Units&&) = delete; // Move construct
Units& operator=(Units const&) = delete; // Copy assign
Units& operator=(Units &&) = delete; // Move assign
I'd drop the Q_DISABLE_COPY and go for the above "= delete" lines, but that's
up for you to decide. You have to add them for move semantics anyway
(Q_DISABLE_COPY does not do that for you).
> units.h:200
> static SharedAppFilter *s_sharedAppFilter;
> + static Units *s_self;
>
Remove this line if you go for the comment i made below.
REPOSITORY
R242 Plasma Frameworks
REVISION DETAIL
https://phabricator.kde.org/D4012
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: broulik, #plasma, davidedmundson, markg
Cc: markg, plasma-devel, #frameworks, lesliezhai, ali-mohamed, jensreuterberg,
abetts, andreaska, sebas
