Hmm, one thing I forgot to ask:

Want me to do a s/Q_NULLPTR/nullptr/, too?

In D3987#77725, @kfunk wrote:

Want me to do a s/Q_NULLPTR/nullptr/, too?

This would make things consistent and avoid confusing future contributors about which one to use, so I'd say yes.

In D3987#77726, @dfaure wrote:

In D3987#77725, @kfunk wrote:

Want me to do a s/Q_NULLPTR/nullptr/, too?

This would make things consistent and avoid confusing future contributors about which one to use, so I'd say yes.

Done.

In D3987#78257, @kossebau wrote:

Seems the porting script had a few wrong matches, where 0 values for enums were misinterpreted as pointer:

E.g.

@@ -1015 +1015 @@ void KWindowSystemPrivateX11::setShowingDesktop(bool showing)
-    NETRootInfo info(QX11Info::connection(), 0, NET::WM2ShowingDesktop);
+    NETRootInfo info(QX11Info::connection(), nullptr, NET::WM2ShowingDesktop);
@@ -1021 +1021 @@ void KWindowSystemPrivateX11::setUserTime(WId win, long time)
-    NETWinInfo info(QX11Info::connection(), win, QX11Info::appRootWindow(), 0, 0);
+    NETWinInfo info(QX11Info::connection(), win, QX11Info::appRootWindow(), nullptr, nullptr);

(snip)

This transformation (0->nullptr) is okay though. Just check the declared QFlags ctor:

Q_DECL_CONSTEXPR inline QFlags(Zero = Q_NULLPTR) Q_DECL_NOTHROW : i(0) {}

This is what we are using now and were using before. Seeing MyFlags(nullptr) may look odd at first, but it's totally fine.

This is what we are using now and were using before. Seeing MyFlags(nullptr) may look odd at first, but it's totally fine.

I agree that it is fine from a compiling POV. But from a human-reading-code POV having a nullptr (thus a pointer) being assigned to a bitflags type as value seems semantically non-sense and needs learning by everyone to accept this exception. More, for unknown variable types one might on first reading think those are pointer types, not flags.

Being odd, being misguiding, more letters to type and read... hm...

So, is there an advantage of using nullptr over 0? :)

I agree. But it's the default value anyway, so why not remove it completely, thus making everyone happy?

In D3987#78383, @dfaure wrote:

I agree. But it's the default value anyway, so why not remove it completely, thus making everyone happy?

What do you mean by "remove"? In the samples from a few comments above, the 0 (or now nullptr) is used as (default) value for an argument:

+    NETWinInfo info(QX11Info::connection(), win, QX11Info::appRootWindow(), nullptr, nullptr);
+    explicit KProgressDialog(QWidget *parent = nullptr, const QString &caption = QString(),
+                             const QString &text = QString(), Qt::WindowFlags flags = nullptr);
+    QCOMPARE(QStringList(queryUrl.queryItems(nullptr).keys()).join(", "),
+                               KLocale::TimeFormatOptions timeOptions = nullptr,

How could that be removed here? Unless you mean replacing with the default constructor of the flags type? That might be even better for human readers at least, agreed :) Though not sure how easy that fix-up is, but maybe the clang artist knows what to do?

Another possible issue with using nullptras value for flags: it might break calling methods with overloads when once a pointer type and once a flags type is used. Not seen, but there is a risk.

void foo(T* t);
void foo(QFlags<Flag> flags);

In D3987#78421, @kossebau wrote:

Another possible issue with using nullptras value for flags: it might break calling methods with overloads when once a pointer type and once a flags type is used. Not seen, but there is a risk.

void foo(T* t);
void foo(QFlags<Flag> flags);

Both foo(0) & foo(nullptr) would call void foo(T* t), no? => No change

In D3987#78420, @kossebau wrote:

In D3987#78383, @dfaure wrote:

I agree. But it's the default value anyway, so why not remove it completely, thus making everyone happy?

What do you mean by "remove"? In the samples from a few comments above, the 0 (or now nullptr) is used as (default) value for an argument:

+    NETWinInfo info(QX11Info::connection(), win, QX11Info::appRootWindow(), nullptr, nullptr);
+    explicit KProgressDialog(QWidget *parent = nullptr, const QString &caption = QString(),
+                             const QString &text = QString(), Qt::WindowFlags flags = nullptr);
+    QCOMPARE(QStringList(queryUrl.queryItems(nullptr).keys()).join(", "),
+                               KLocale::TimeFormatOptions timeOptions = nullptr,

How could that be removed here? Unless you mean replacing with the default constructor of the flags type? That might be even better for human readers at least, agreed :) Though not sure how easy that fix-up is, but maybe the clang artist knows what to do?

Qt::WindowFlags flags = {}, or
Qt::WindowFlags flags = Qt::WindowFlags()

In D3987#78426, @kfunk wrote:

In D3987#78421, @kossebau wrote:

void foo(T* t);
void foo(QFlags<Flag> flags);

Both foo(0) & foo(nullptr) would call void foo(T* t), no? => No change

One second after I clicked "Submit" I thought "meh, should have checked before if that really is the case" ;) Might be not indeed. Perhaps this might also explain why there is this otherwise (to me) strange nullptr default value with the QFlags constructor, as it will avoid ambiguity WRT implicit conversion.

So allow me to reduce that remark to: if one adds an overload with a pointer argument in an API, all consumer of that API will not notice that suddenly all their calls switch to the pointer overload. Does that hold?

Update: the consumer would also not notice with 0 (being equal here as assumed above), but they will have a more hard time to find all places when other people already added code which really wanted to use the new overload with a null pointer.
So just a corner case, but it is one :)

In D3987#78427, @kfunk wrote:

In D3987#78420, @kossebau wrote:

In D3987#78383, @dfaure wrote:

I agree. But it's the default value anyway, so why not remove it completely, thus making everyone happy?

What do you mean by "remove"? In the samples from a few comments above, the 0 (or now nullptr) is used as (default) value for an argument:

+    NETWinInfo info(QX11Info::connection(), win, QX11Info::appRootWindow(), nullptr, nullptr);
+    explicit KProgressDialog(QWidget *parent = nullptr, const QString &caption = QString(),
+                             const QString &text = QString(), Qt::WindowFlags flags = nullptr);
+    QCOMPARE(QStringList(queryUrl.queryItems(nullptr).keys()).join(", "),
+                               KLocale::TimeFormatOptions timeOptions = nullptr,

How could that be removed here? Unless you mean replacing with the default constructor of the flags type? That might be even better for human readers at least, agreed :) Though not sure how easy that fix-up is, but maybe the clang artist knows what to do?

Qt::WindowFlags flags = {}, or
Qt::WindowFlags flags = Qt::WindowFlags()

"What to do" was about how to create a diff automatically :)

Have to say, personally I am used to the 0 to denote a bitmask without any bit set, and would assume quite some other people as well. Not sure if {} would be an improvement for the human reader, being another pattern to learn.

But assuming we want to avoid 0 as value for QFlags-based types, when comparing

NETWinInfo info(QX11Info::connection(), win, QX11Info::appRootWindow(), {}, {});
explicit KProgressDialog(QWidget *parent = nullptr, const QString &caption = QString(),
                         const QString &text = QString(), Qt::WindowFlags flags = {});
QCOMPARE(QStringList(queryUrl.queryItems({}).keys()).join(", "),
                           KLocale::TimeFormatOptions timeOptions = {},

with

NETWinInfo info(QX11Info::connection(), win, QX11Info::appRootWindow(), NET::Properties(), NET::Properties2());
explicit KProgressDialog(QWidget *parent = nullptr, const QString &caption = QString(),
                         const QString &text = QString(), Qt::WindowFlags flags = Qt::WindowFlags());
QCOMPARE(QStringList(queryUrl.queryItems(KUrl::QueryItemsOptions()).keys()).join(", "),
                           KLocale::TimeFormatOptions timeOptions = KLocale::TimeFormatOptions(),

it might be nice to use {} for default values, as the type is given already, and the explicit constructor for arguments when calling methods, as it adds information in the code itself.

My 2 cents. In the end I was okay with 0, just am unhappy with nullptr being used for QFlags values, for the reasons given :)

Seems the nullptr for QFlags values also breaks Python binding generation:

Attempting to build the python bindings now fails because our scripting to
generate sip files (or sip itself - I didn't investigate further because it
seems obvious that the headers should be fixed to solve the issue) can not
handle this.

From https://mail.kde.org/pipermail/kde-frameworks-devel/2017-January/041984.html

{} sounds like the best solution to me.

In D3987#78468, @dfaure wrote:

{} sounds like the best solution to me.

I think that should be used instead of nullptr anyway.

void foo(const QModelIndex& idx = {});

is obviously better than

void foo(const QModelIndex& idx = QModelIndex());

and using = {} in all cases for consistency is desirable IMO.

So seems everyone so far agrees that {} is best for the default value.

What about the passed argument case? Let's look at the two respective samples:

NETWinInfo info(QX11Info::connection(), win, QX11Info::appRootWindow(), {}, {});
QCOMPARE(QStringList(queryUrl.queryItems({}).keys()).join(", "),

vs.

NETWinInfo info(QX11Info::connection(), win, QX11Info::appRootWindow(), NET::Properties(), NET::Properties2());
QCOMPARE(QStringList(queryUrl.queryItems(KUrl::QueryItemsOptions()).keys()).join(", "),

As said before, here IMO the explicit constructor (NET::Properties(), NET::Properties2(), KUrl::QueryItemsOptions()) call has advantages over {}, as it make it clear to the reader what is passed. Would also help against ambiguity on overloads. It would be consistent how e..g QString() is used in many methods calls.
More to type, but it helps reading the code a lot (and you should use a proper editor/IDE with auto-completion ;) ).

It's true that QFlags has a ctor taking a nullptr. The bindings-related problem was in our handling of enums by prepending a namespace, so we would end up with Qt::nullptr. I've fixed that in ECM, and also fixed handling of = {} for various types too.

So, at least from a bindings POV, the problem is solved.

The signature of KUrl::queryItems is
queryItems(const QueryItemsOptions &options = 0),
i.e. = {}, so the caller should just not pass any value ;-)

But otherwise I agree, being explicit on the calling side is the best option.

Closing. This Diff refactored the code technically correct.

If you want to replace MyFlags flags = nullptr by ... = {} please do so in a follow-up patch/review.

In D3987#95858, @kfunk wrote:

Closing. This Diff refactored the code technically correct.

Technically correct, as in: it builds.
But semantically it is incorrect and a regression when it comes to flags, especially as high level languages are made for humans in the first line, not compilers ;)

If you want to replace MyFlags flags = nullptr by ... = {} please do so in a follow-up patch/review.

Seems you do not see yourself in the duty to do the fix-up. so I have to scratch this itch myself, tss :)
So please tell what tools you used to create this diff, so I could have a look how I can reuse them for fixing all the MyFlags flags = nullptr by ... = {}? Any suggestions how that could be done best?

In D3987#96418, @kossebau wrote:

In D3987#95858, @kfunk wrote:

Closing. This Diff refactored the code technically correct.

Technically correct, as in: it builds.
But semantically it is incorrect and a regression when it comes to flags, especially as high level languages are made for humans in the first line, not compilers ;)

Not true, the patch does /not/ change behavior. See explanation in https://phabricator.kde.org/D3987#78426.

If you want to replace MyFlags flags = nullptr by ... = {} please do so in a follow-up patch/review.

Seems you do not see yourself in the duty to do the fix-up. so I have to scratch this itch myself, tss :)

Yes, honestly I think either way is fine (nullptr or {}). Get... used to the new look I'd say ;)

So please tell what tools you used to create this diff, so I could have a look how I can reuse them for fixing all the MyFlags flags = nullptr by ... = {}? Any suggestions how that could be done best?

clang-tidy was being used. You need to special case its 'modernize-use-nullptr' check for QFlags, fwiw...

I just found out that this change introduced wrong replacements.
The changes in frameworks/kimageformats/src/imageformats like the following are incorrect, since the return type is not a pointer but an enum flags type, so it should still be 0, not nullptr.

@@ -337,10 +337,10 @@ QImageIOPlugin::Capabilities EPSPlugin::capabilities(QIODevice *device, const QB

    return Capabilities(CanRead | CanWrite);
}
if (!format.isEmpty()) {

• return 0;

+ return nullptr;

}
if (!device->isOpen()) {

• return 0;

+ return nullptr;

}
 
Capabilities cap;

In D3987#174960, @mkoller wrote:

I just found out that this change introduced wrong replacements.

It did not, the changes are correct, though they're probably not what everyone expects.

Please read through the comments in this Diff, this issue has been discussed in length already.

tl;dr: Proposed resolution of this "problem": I'd replace return nullptr in your cases with return {} or just leave it as-is.

The changes in frameworks/kimageformats/src/imageformats like the following are incorrect, since the return type is not a pointer but an enum flags type, so it should still be 0, not nullptr.

@@ -337,10 +337,10 @@ QImageIOPlugin::Capabilities EPSPlugin::capabilities(QIODevice *device, const QB

    return Capabilities(CanRead | CanWrite);
}
if (!format.isEmpty()) {

• return 0; + return nullptr; } if (!device->isOpen()) {
• return 0; + return nullptr; }
  
  Capabilities cap;