Agreed about both.

Not strictly limited to KF6 even, changing d-pointer types can be done at any time as long as we don't change the size of the public class, and assuming there is no inline code using d (which there very likely isn't, that would defeat the point of the d-pointer in the first place).

Nested private classes have the error-prone downside that they inherit the export specification from the outer class, so they become exported symbols (which we explicitly don't want), thus needing an explicit Q_DECL_HIDDEN.

Using std::unique_ptr has the additional advantage of making classes easily movable where necessary.

So I do agree on both changes as well obviously :)

And @kossebau said he agrees on IRC (which encouraged me to create the task :)).

i've no problem either way (and also lean towards unique_ptr), but would like to see this done sweeping on a per-framework basis (that is. until a framework is moved wholesale to using that, it would be nice not to have two different styles of dptr implementation in the same framework, it's a bit jarring to come across).

Would it make sense encourage the use of the Qt Macros to also have some uniformity in this way or is it not needed?

Which Macros do you have in mind?

The Q_DECLARE_PRIVATE, Q_DECLARE_PUBLIC and Q_Q and Q_D

We wouldn't need Q_Q and Q_D since we'd have this in the header:
private:

std::unique_ptr<ClassFooPrivate> d;

and in the definition of ClassFooPrivate:

ClassFoo *const q

then we can use d->blah() and q->bleh() without Q_Q and Q_D. At least that's the pattern I see in recent KDE code.

Neither would we need Q_DECLARE_PRIVATE/PUBLIC since ClassFooPrivate isn't nested (i.e. not ClassFoo::Private); it's explained much better here https://mail.kde.org/pipermail/kde-frameworks-devel/2015-August/025956.html

You asked a very useful question, I should have included that in my first post. :)

Thanks for the explanation!

The main point of the Qt macros is to have the Private of the subclass inherit from the one of the base class, so that a single allocation is necessary.
The syntax of the Qt macros is to encapsulate that.

This is useful in some frameworks like KArchive which have a class inheritance within the framework.
Actually the KIO jobs do that too.
class SimpleJobPrivate: public JobPrivate
etc.

In summary, no, don't use the Qt macros everywhere, but they are very useful for the case of class hierarchies within a framework.

Another issue that was raised on IRC, is the usefulness of the Q_D macro when dealing with const member methods of ClassFoo, since d is a private member of ClassFoo, it's const in const methods, but if it's derefrenced then the object it points to, ClassFooPrivate isn't const. With Q_D one would have to use:
Q_D(const ClassFoo)

in const member methods. (Sorry if there are any logic issues with the above, I am still trying to wrap my head around all that Qt d-ptr "opaque" stuff :))

How is this "an issue with usefulness"? It's just part of the syntax. If you forget the const, the compiler reminds you.

error: invalid conversion from ‘const KIO::SlavePrivate*’ to ‘KIO::SlavePrivate*’ [-fpermissive]

I meant when not using the Q_D macro, e.g. in:

qint64 KIO::ApplicationLauncherJob::pid() const
{
    return d->m_pids.at(0);
}

the compilation wouldn't fail if I added this to that method:

(*d).m_numProcessesPending = 10;

whereas with Q_D one would need to Q_D(const ApplicationLauncherJobPrivate), and then something like that assignment wouldn't work.

EDIT: the very last paragraph in https://wiki.qt.io/D-Pointer#The_d-pointer (I was searching for "const", too many results...):

The d_func also has the advantage to enforce const-correctness: In a const member function of MyClass you need a Q_D(const MyClass) and thus you can only call const member functions in MyClassPrivate. With a naked d_ptr you could also call non-const functions.

OK. IMHO that's a nice side-effect but not a good enough reason to use Q_D everywhere. It confuses C++ developers who haven't read Qt code before, raising the barrier of entry for contributions.
To save allocations in case of inheritance it's worth it, but just for const correctness, I'm not fully convinced.
Just my 2 cents.

I haven't seen the d-pointer dereferenced before (at all?), usually one accesses the private class via the d-poitner, rather than deref'ing or get().

(It means one is able to use d->something() or q->something() directly without having to remember to add Q_Q/D at the top of the method).

(Updated the original task based on the feedback so far)

You can't use d->something() when using the Qt macros, without first doing Q_D() which declares a local variable d from the result of d_func().

Try removing the Q_D line from CopyJob::doSuspend to see what I mean.

Maybe I misunderstood your line about I haven't seen the d-pointer dereferenced before (at all?), usually one accesses the private class via the d-poitner, to me that's the same.

BTW "dereferencing" a pointer is simply doing p->foo().

In T13924#246239, @dfaure wrote:

Maybe I misunderstood your line about I haven't seen the d-pointer dereferenced before (at all?), usually one accesses the private class via the d-poitner, to me that's the same.

BTW "dereferencing" a pointer is simply doing p->foo().

Indeed, -> operator is the same as (*p).foo() (that's 100001 C++... sorry :)).

In T13924#246232, @dfaure wrote:

OK. IMHO that's a nice side-effect but not a good enough reason to use Q_D everywhere. It confuses C++ developers who haven't read Qt code before, raising the barrier of entry for contributions.

I remember from my first days of C++ this was one of the first things I learned, I do not think it is such a barrier, it actually made straight sense. C++ has other stuff which is way more complicated IMHO ;) I have seen that pattern to restore constness for the members in non-Qt-code as well and also remember it being described/teached as something to do part of normal PIMPL.

I think especially with Qt and all its implicitly shared data containers we really want the compiler help us to make sure to only call const methods as intended.

I'm sure you learned about pimpl yes, but not about Q_D/Q_Q macros and Private classes inheriting from each other...

But I agree, forwarding constness is important.

In T13924#247309, @dfaure wrote:

I'm sure you learned about pimpl yes, but not about Q_D/Q_Q macros and Private classes inheriting from each other...

Given it is over a decade ago, surely my memory might trick me a lot :) But I would bet it was around the same initial years when I was discovering C++ and also exploring Qt sources to learn more best practices and saw matching things . A related milestone I remember well is 2010's article from Marc (https://m.heise.de/developer/artikel/C-Vor-und-Nachteile-des-d-Zeiger-Idioms-Teil-2-1136104.html?seite=all) where reading that I felt like being an expert due to knowing well all the stuff mentioned in the article :P So in my small bubble that seemed professional C++ standard knowledge.,

On the other hand indeed in code across KDE projects those patterns are not used that much, mostly only found in older code, so I might have been a fashion at those times and lesser known these days. Which I understand, all those macro magic needed requires more code discipline and makes the code less pure. In my dreams future C++ specifications would support that pattern out of the box...

OK, you're a bit of a "special case" then. Most developers who want to make their first contribution to KDE, do not start by reading Qt sources (nor Marc's 2010 article, lol). Most people initially treat Qt as a black box, relying on its documentation only.

What's the point of using std::unique_ptr here? Isn't that inherent in the fact that the instance is private and (typically) allocated in the dtor?

And FWIW code that wants to maintain compatibility with older Qt5 versions can't use this way of declaring the d-ptr, e.g. (backporting a patch):

In file included from /opt/local/include/qt5/QtCore/qnamespace.h:43:
/opt/local/include/qt5/QtCore/qglobal.h:1008:112: error: no member named 'data' in 'std::unique_ptr<KToolTipHelperPrivate, std::default_delete<KToolTipHelperPrivate> >'
template <typename Wrapper> static inline typename Wrapper::pointer qGetPtrHelper(const Wrapper &p) { return p.data(); }
                                                                                                             ~ ^
/.../src/ktooltiphelper.h:76:5: note: in instantiation of function template specialization 'qGetPtrHelper<std::unique_ptr<KToolTipHelperPrivate, std::default_delete<KToolTipHelperPrivate> > >' requested here
    Q_DECLARE_PRIVATE(KToolTipHelper)
    ^
/opt/local/include/qt5/QtCore/qglobal.h:1011:81: note: expanded from macro 'Q_DECLARE_PRIVATE'
    inline Class##Private* d_func() { return reinterpret_cast<Class##Private *>(qGetPtrHelper(d_ptr)); } \

And FWIW code that wants to maintain compatibility with older Qt5 versions can't use this way of declaring the d-ptr, e.g. (backporting a patch):

When we bump minimum versions we want to use the things they have, if we take care to not break older versions there is no point in requiring newer versions

When backporting patches I would assume they usualy do not touch the declaration of the private class instance...

When we bump minimum versions we want to use the things they have, if we  take care to not break older versions there is no point in requiring newer versions

My point: code that wants to maintain compatibility can't align itself to this new idiom - but it could if a QScopedPointer were/had been chosen.

When backporting patches I would assume they usualy do not touch the declaration of the private class instance...

Hah, do you also assume they wouldn't contain one? ;)

Which version of Qt5 is that? currently we require 5.14.0.

Ahmad Samir wrote on 20210316::13:50:33 re: "T13924: Unify how d-pointer is created in frameworks' classes"

Which version of Qt5 is that? currently we require 5.14.0.

That's 5.9.8; the "correct" version was introduced somewhere between there and 5.12.6 .

The current version of the frameworks requires Qt 5.14.0, but not every application under active development requires the latest frameworks and/or Qt versions (and a good thing that is too, IMHO).

In T13924#251410, @rjvbb wrote:

What's the point of using std::unique_ptr here?

The point is to use the expressiveness of this highlevel language feature to tell everyone: this instance here is just owned by this very class object, and should be bound to the lifetime of the class object. Which is better than having manual code just following a pattern.

And FWIW code that wants to maintain compatibility with older Qt5 versions

This code wants to maintain compatibility with the very Qt versions as defined in the toplevel CMakeLists.txt, which is currently >= 5.14. If someone wants to take code and make it work with older versions, bad luck for them, they have to do more work.

can't use this way of declaring the d-ptr, e.g. (backporting a patch):

You can by doing the extra step in your backporting and change from std::unique_ptr to QScopedPointer to have Qt's PIMPL macro work with the older variant of qGetPtrHelper that relies on data() and not operator->().
Or patch your Qt and backport 98ef4239a6b859522a45bdd03ba5496ea201e5f3 to it, as you will run into more recent code using smart pointer types to lifetime-manage the pimpl object.

The point is to use the expressiveness of this highlevel language feature to tell everyone: this instance here is just owned by this very class object,

You mean to confirm to anyone who had forgotten what `private' means? 8-)

and should be bound to the lifetime of the class object. Which is better than having manual code just following a pattern.

What can I say ... better late than never (and have fun)?

When/where does the actual allocation of the private class instance take place. Just before the ctor is entered I presume, so its definition can be kept private and code that creates an instance of the parent class on the heap doesn't have to be rebuilt if the private class changes?

Or patch your Qt and backport 98ef4239a6b859522a45bdd03ba5496ea201e5f3 to it, as you will run into more recent code using smart pointer types to lifetime-manage the pimpl object.

Considered that and then lamented that C++ doesn't allow redefining a template.

In T13924#251425, @rjvbb wrote:

The point is to use the expressiveness of this highlevel language feature to tell everyone: this instance here is just owned by this very class object,

You mean to confirm to anyone who had forgotten what `private' means? 8-)

This is an orthogonal concept to private, I cannot follow what you mean here.

Considered that and then lamented that C++ doesn't allow redefining a template.

If that is not possible (no clue if it is and do not have time left to help debug this) then it is plan B: patch the backport to use QScopedPointer.

(Or plan C: update Qt to some more recent version. If you are stuck with some old Qt for whatever reason, you have to deal with its feature set of that time, and so you will have to do the same with KDE Frameworks, which is an extension to Qt by its own product definition).

Back to your initial question: current Frameworks code is to be written with the officially supported dependencies in mind and then also future-proof where foreseeable. std::unique_ptr is a well-known part of the official C++ standard library and will be, QScopedPointer is some old custom Qt approach at the time when std::unique_ptr was not yet available. So the choice has been obvious.
And using a smart pointer over manual code is an improvement as well, as things are better defined this way, avoiding some human errors.

Considered that and then lamented that C++ doesn't allow redefining a template.

off topic but you could specialize the template for unique_ptr

this instance here is just owned by this very class object,

>
> You mean to confirm to anyone who had forgotten what `private' means? 8-)

This is an orthogonal concept to private, I cannot follow what you mean here.

Unless I'm mistaken, a private class member can't (shouldn't) be owned by anyone else.

> Considered that and then lamented that C++ doesn't allow redefining a template.

off topic but you could specialize the template for unique_ptr

Ah, right, and you could probably do that with a macro to keep the syntax generic. Fun! :)

Don't heed my any further, carry on!

> Considered that and then lamented that C++ doesn't allow redefining a template.

off topic but you could specialize the template for unique_ptr

Ah, right, and you could probably do that with a macro to keep the syntax generic. Fun! :)

No need for a macro even: https://invent.kde.org/libraries/kpublictransport/-/blob/master/src/lib/pointer_helper_p.h#L14 - that gives you the backward compatibility with older Qt versions without conflicting with the newer ones.

No need for a macro even: https://invent.kde.org/libraries/kpublictransport/-/blob/master/src/lib/pointer_helper_p.h#L14 - that gives you the backward compatibility with older Qt versions without conflicting with the newer ones.

Doh, of course, thanks!

(for the record, a working link for pointer_helper_p.h is https://invent.kde.org/libraries/kpublictransport/-/blob/release/20.12/src/lib/pointer_helper_p.h given that it has been removed in master)

Recently I ran into a weird startup issue that ultimately I traced back to the use of std::unique_ptr and its underlying implementation. It's been a while, so I'm no longer fresh on the details. Basically, if an interrupt or exception occurs during the call to std::unique_ptr, a subsequent call will hang (or crash). From what I understood it's mostly an issue on non-x86_64 platforms, but it can happen there too (as I found out). There has been at least 1 attempt to fix the problem in the underlying system library (libstdc++, I think but possibly libc) but that one was reverted because it introduced other regressions, from what I understand.

I could dig up a link or too if you want, and share the workaround I found.

My two cents: We can obtain const correctness also without of Q_D and Q_Q, but with real pointers. Personally, I'm doing this for unique pointers (as d pointers) and also for raw pointers (as q pointers). In both cases it’s a quite lightwise header providing a template that behaves like the original unique pointer or raw pointer, but with const correctness. There are more sophisticated implementations out there on the Internet, but this one was enough for my use case. Maybe this code could help in KDE?