Pushed new version 1

Pushed new version 1

NAME is used for both the class and the instance name? I foresee interesting compilation messages when NAME is used wrongly :)

Also – I'd feel better if you could change the “class NAME” to “struct NAME” so that it’s clearer that it’s a POD without constructors / destructors. And a big comment saying “NEVER EVER ADD A CONSTRUCTOR OR DESTRUCTOR, AND DON’T ADD ANY DATA MEMBERS” wouldn’t hurt either ;)

The old Q_GLOBAL_STATIC expanded to a global static function, which the compiler might or might not want to inline. The operator-> in your version is inlined, and contains a lot of code – can we out-of-line it?

My last wish – to be consistent, I'd also add the “inline” keyword to destroy() and operator().

Yes NAME is used for both, any suggestions on how to do this differently? If the class is called GlobalStatic_##NAME, then users of the class need to know that, so that they can write
friend class GlobalStatic_Foo;
if their global static “Foo” needs to call a private constructor.

This is also why I changed it from struct to class, otherwise they would have to write friend struct . (On Windows you can’t mix and match, the friend declaration has to be precise)

About constructor and destructor in the generated class: why would we ever want to do that? This would happen far too early (at lib load)… I’ll add a comment to make you happy, but there was no use case for this anyway, IMHO.

Outlining operator-> : excellent idea. Done.
Inline for destroy() and operator(): OK, done.

So the only issue left is the name of the class (I'm very open to suggestions, the current solution has an issue with uppercase vs lowercase: Q_GLOBAL_STATIC(QIconLoader, iconLoaderInstance) needs “friend class iconLoaderInstance;” whereas a titlecase class name would be expected. My only idea is really GlobalStatic_iconLoaderInstance, and documenting that hardcoded classname…

Interestingly, outlining operator-> triggers a link error when two .cpp files name a global static the same way:

/d/qt/4/qt5/qtbase/src/corelib/tools/qregexp.cpp:3675: multiple definition of `mutex::operator->()‘
/d/qt/4/qt5/qtbase/src/corelib/thread/qthreadstorage.cpp:75: first defined here

But I think this only shows that these two cpp files should use a different name for their global static. I’ll change them, if you agree.

About constructor and destructor in the generated class: why would we ever want to do that?

The trick is that we can’t do that, otherwise we’ll get interesting side effects. And if we should never ever do something, it would be nice to make that explicit with a comment.

Interestingly, outlining operator-> triggers a link error when two .cpp files name a global static the same way:

Hmmm… Is the Q_GLOBAL_STATIC ever going to be used outside the current *.cpp file? I don’t think so, so maybe putting the whole thing in an anonymous namespace might fix the issue?

The anonymous namespace would break the friend declaration idea. Let me expand on that idea:

It doesn’t seem to happen in Qt, but apps very often have public singletons. Like:

class MySingleton
{
public:
static MySingleton *self();
};

Now the problem is that the constructor for MySingleton should be private, so that the rest of the app can’t call it. But still the NAME class must be able to call it. So you need a friend declaration, right there, in the public header file. So “NAME” can’t be in the anonymous namespace, or have a weird name. It must have a name that people can find out, and can use for writing the friend declaration. Something like GlobalStatic_instance, assuming Q_GLOBAL_STATIC(MySingleton, instance). But this might still clash if another file says Q_GLOBAL_STATIC(WhatEver, instance), so better use more descriptive names in that macro :–)

Ouch, 15 times Q_GLOBAL_STATIC_WITH_ARGS(QFactoryLoader, loader, …) in QtGui…

doesn’t this mean that the symbols become visible to an other compilation units?

13760a6: there are an extra/unrelated/unwanted files in the commit (i.e. src/uitools/qtuitoolsversion.h, src/users.list)

(and I still see no point in having two unrelated commits in here)

Becoming visible to other compilation units is the only way to make the “friend class” trick work, isn’t it?

I can see the downside in doing that, but otherwise writing singletons is really annoying – e.g. in KDE we currently need to write another struct, use that in the global static macro, have an instance of the real class as member, and declare the struct as a friend to the class —– very cumbersome.

Maybe we need to make it an option, whether to use an anonymous struct or to generate a non-internal class…

Thanks for spotting the unwanted files, git mistake. The unrelated commits were because someone at Nokia will have to pick the commits from here and put them into gerrit anyway, so I thought he could do that at the same time, but OK, I moved these two commits to merge request number 4.

I’ll update this MR once we decide on something about the generated class.

Pushed new version 2

in KDE we currently need to write another struct, use that in the global static macro, have an instance of the real class as member, and declare the struct as a friend to the class

did you mean something like this:

-- singleton.h
class Singleton
{
public:
    static Singleton* instance();

    ~Singleton();

    void doSomething();

protected:
    Singleton();
};

-- singleton.cpp
class SingletonPrivate : public Singleton
{
public:
    SingletonPrivate() : Singleton() {}
};
Q_GLOBAL_STATIC(SingletonPrivate, theInstance);

Singleton* Singleton::instance()
{
    return theInstance();
}

Singleton::Singleton()
{
}

Singleton::~Singleton()
{
}

void Singleton::doSomething()
{
    qDebug("blah-blah");
}

? it doesn't sound too complicated/inconvenient to me...
an other way to do the same is:

-- singleton.h
class Singleton
{
public:
    static void doSomething();
};

-- singleton.cpp
class SingletonPrivate
{
public:
    void doSomething() { qDebug("blah-blah"); }
};

Q_GLOBAL_STATIC(SingletonPrivate, theInstance);

void Singleton::doSomething()
{
    theInstance()->doSomething();
}

which looks nicer and shorter to me and disallows the stupid user to destroy the singleton class instance.

also note that both snippets were taken from the real use-cases and both of them doesn't require the friend declaration. but even if you need to make it someone's friend, you know it's name for sure.

I'm sure you know all these, just wanted to say I'm quite happy with the current implementation. though, isDestroyed()/destroy() methods are features I'd want to have in some rare/corner cases (like an ordered instances destruction, etc.).
in my opinion, it would be cool to have them as an *optional* addition to the existing implementation. documenting these macroses would be also great indeed.

Deriving from the singleton class is a cool trick, I hadn’t thought of that :) However it requires that the constructor is protected, which is not wanted in public API (people could inherit from the class to call that constructor).
Case in point: QSystemLocale, the (bool) ctor is private, and the global static is a friend. This case fits into neither of the two examples you gave — but can be solved with an extra class just to mark it that class as a friend.

—– a/src/corelib/tools/qlocale.cpp
+++ b/src/corelib/tools/qlocale.cpp
@@ -75,7 +75,13 @@ void qt_symbianInitSystemLocale();

#ifndef QT_NO_SYSTEMLOCALE
static QSystemLocale _systemLocale = 0;
-Q_GLOBAL_STATIC_WITH_ARGS(QSystemLocale, QSystemLocale_globalSystemLocale, (true))
+class QSystemLocaleGlobal
+{
+public:
+ QSystemLocaleGlobal() : instance(true) {}
+ QSystemLocale instance;
+};
+Q_GLOBAL_STATIC(QSystemLocaleGlobal, globalSystemLocale)
static QLocalePrivate system_lp = 0;
Q_GLOBAL_STATIC(QLocalePrivate, globalLocalePrivate)
#endif
@@ -467,7 +473,7 @@ static const QSystemLocale *systemLocale()
#if defined(Q_OS_SYMBIAN)

 qt_symbianInitSystemLocale();

#endif
– return QSystemLocale_globalSystemLocale();
+ return &globalSystemLocale->instance;
}

void QLocalePrivate::updateSystemPrivate()
diff —git a/src/corelib/tools/qlocale.h b/src/corelib/tools/qlocale.h
—– a/src/corelib/tools/qlocale.h
+++ b/src/corelib/tools/qlocale.h
@@ -123,7 +123,7 @@ public:

private:

 QSystemLocale(bool);

• friend class QSystemLocale_globalSystemLocale;
  
• friend class QSystemLocaleGlobal;
  };
  

  endif

That’s what I was trying to avoid, but apparently the price is too high (symbols clashing between compilation units), so let’s go with that.

Re destroy: Not sure what you meant by optional — I think multiple macros just for this would only confuse people, and IMHO it can’t hurt to have generated isDestroyed/destroy method even if not used — they are only visible in the .cpp file anyway.

I'd prefer

-- cpp
Q_GLOBAL_STATIC(QSystemLocale, globalSystemLocale)  
..  
    return globalSystemLocale();  
}

-- h  
class QSystemLocale;  
..  
friend static QSystemLocale *globalSystemLocale();

is this requires not more efforts than

-- h
friend class QSystemLocale_globalSystemLocale;

but without those naming dances and symbols clashing

and IMHO it can’t hurt to have generated isDestroyed/destroy method even if not used — they are only visible in the .cpp file anyway.
true. but unless it is not inlined, there would be a warning about unused function in 99.9% of cases.
well, nevermind. I don’t see a possibility to implement such a feature via static function (the QGlobalStatic is local static inside the relevant local static function)

I think you didn’t realize that the new Q_GLOBAL_STATIC generates a whole class, not a function.

isDestroyed() and destroy() are class members, not global functions. Which means, no warnings if they are unused.

And this is also why “friend static QSystemLocale *globalSystemLocale()” doesn’t work anymore, which is why I was looking for alternative solutions (marking the generated class as friend, but this creates clashes, so I'm changing my mind about that).

so, it seems like you missed my point… I was talking about the existing implementation.

I'm putting this on hold for now — Thiago said he would write a better solution, where the constructor can only ever be called once, based on atomic operations (like in his Q_ONCE test code).

Thank you for your contribution. With the launch of the Qt Project
under Open Governance [1], you can now contribute your changes via Gerrit [2].
At the same time, contributions via Gitorious can no longer be merged.

We would still like to see your patch on the new platform, and wish for you to become
a member of the Qt Project. Please read [3] and in particular [4] to familiarize yourself
with the new tools. You will still find a read-only mirror of all Qt repositories on
Gitorious.

With a much wider set of possible reviewers, we are also looking forward to better
response times to contributions. Looking forward to your patch!

The Nokia Qt team

[1] http://labs.qt.nokia.com/2011/10/21/the-qt-project-is-live/
[2] http://codereview.qt-project.org
[3] http://www.qt-project.org
[4] http://developer.qt.nokia.com/wiki/Gerrit_Introduction