[Differential] [Requested Changes To] D4012: Introduce Units singleton

[markg (Mark Gaiser)]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20170108/44f5f64c/attachment-0001.html>