Use nullptr in all Frameworks (just diff in KIO shown here)
ClosedPublic

Authored by kfunk on Jan 5 2017, 11:44 PM.

Details

Reviewers
dfaure
kossebau
Group Reviewers
Frameworks
Commits
R308:216e183ed67a: Use nullptr everywhere
R246:2a7820493508: Use nullptr everywhere
R268:8e6a623381cd: Use nullptr everywhere
R274:29af8bd18d61: Use nullptr everywhere
R315:8ddf6a02b42a: Use nullptr everywhere
R241:fecf8fbc7348: Use nullptr everywhere
R276:82fc0fa06a62: Use nullptr everywhere
R265:03d14c2fbf1c: Use nullptr everywhere
R270:a408ca1469b6: Use nullptr everywhere
R279:ed6f50c8494d: Use nullptr everywhere
R249:3499bfb96cab: Use nullptr everywhere
R305:f51dfa3c6a3f: Use nullptr everywhere
R285:55ccfdb8751f: Use nullptr everywhere
R301:8abf0791c79c: Use nullptr everywhere
R297:7a616ec8f7d8: Use nullptr everywhere
R272:f75b735ddd99: Use nullptr everywhere
R291:cad3bceaec52: Use nullptr everywhere
R282:41b0e0b0811c: Use nullptr everywhere
R39:efa7e9566985: Use nullptr everywhere
R238:272e91c45c2b: Use nullptr everywhere
R307:c0e742f29a0e: Use nullptr everywhere
R303:e21ec093b15d: Use nullptr everywhere
R289:dcf0b65a8d93: Use nullptr everywhere
R127:f993459847c8: Use nullptr everywhere
R6:51318fa18f9f: Use nullptr everywhere
R288:990cdab6b587: Use nullptr everywhere
R298:80646e91e072: Use nullptr everywhere
R235:b13dd72a3151: Use nullptr everywhere
R284:08d7d142d392: Use nullptr everywhere
R275:1c02848a4d76: Use nullptr everywhere
R281:dd64e31e3be8: Use nullptr everywhere
R245:4c9913726091: Use nullptr everywhere
R300:e80fa26c411f: Use nullptr everywhere
R273:cb82278f0b4d: Use nullptr everywhere
R312:1c17b1139499: Use nullptr everywhere
R295:9e10b60d0c77: Use nullptr everywhere
R236:01a16eff2ea8: Use nullptr everywhere
R313:3990f88b2ab9: Use nullptr everywhere
R271:11071d952962: Use nullptr everywhere
R294:9ce84a3a7923: Use nullptr everywhere
R302:16c808a0d2b6: Use nullptr everywhere
R237:f83b4b191d62: Use nullptr everywhere
R314:3cb1f08515dd: Use nullptr everywhere
R252:f8bf757b0581: Use nullptr everywhere
R306:90928eb46d66: Use nullptr everywhere
R283:d388e3c48d55: Use nullptr everywhere
R299:c4c17d5f1cfd: Use nullptr everywhere
R293:84358f26a51e: Use nullptr everywhere
R277:948bdf64b9b0: Use nullptr everywhere
R310:4eb51e59fd1e: Use nullptr everywhere
R317:754d0f700d7d: Use nullptr everywhere
R292:08133835c181: Use nullptr everywhere
R280:42ae697a1afe: Use nullptr everywhere
R243:f07549376fbb: Use nullptr everywhere
R296:a4703979d96a: Use nullptr everywhere
R316:de4af02b70a7: Use nullptr everywhere
R269:780a955cb3d5: Use nullptr everywhere
R239:cba0b3cf1ca9: Use nullptr everywhere
R304:fcd97c603069: Use nullptr everywhere
R311:a6e1292ffa34: Use nullptr everywhere
R290:0068052e01fc: Use nullptr everywhere
R244:41031049c848: Use nullptr everywhere
R287:740fe5df0e9c: Use nullptr everywhere
R278:b06bb3ac8088: Use nullptr everywhere
R309:9721a8efaff4: Use nullptr everywhere
R263:8c82baf8aa6f: Use nullptr everywhere
Summary

The full patch (all Frameworks ported to using nullptr instead of null literals) changes around 9000 lines in total (see here:

):

This is what I use locally to keep track of my changes:

% kde-frameworks-list.sh  | xargs -n1 -I% sh -c "(cd %; git-difflinesonly.sh)" | head
-Code39Barcode::Code39Barcode() : AbstractBarcode(), d(0){
+Code39Barcode::Code39Barcode() : AbstractBarcode(), d(nullptr){
-Code93Barcode::Code93Barcode() : AbstractBarcode(), d(0){
+Code93Barcode::Code93Barcode() : AbstractBarcode(), d(nullptr){
-DataMatrixBarcode::DataMatrixBarcode() : d(0) {
+DataMatrixBarcode::DataMatrixBarcode() : d(nullptr) {
-QRCodeBarcode::QRCodeBarcode() : AbstractBarcode(), d(0){
+QRCodeBarcode::QRCodeBarcode() : AbstractBarcode(), d(nullptr){
-    BarcodeExampleWidget(Prison::AbstractBarcode* barcode, QWidget* parent=0);
+    BarcodeExampleWidget(Prison::AbstractBarcode* barcode, QWidget* parent=nullptr);


% kde-frameworks-list.sh  | xargs -n1 -I% sh -c "(cd %; git-difflinesonly.sh)" | wc -l
18592

-> ~9000 lines changed.

This change affects *all files*. Not just headers.

There are more options to limit the number of changes:

  • Less changes: Just change headers (.h files) -- easy
  • Even less changes: Just change public headers -- slightly more difficult for me to figure out *what* is public from a scripting POV

If you think we should limit our changes, please speak up. I wouldn't recommend it though. Let's move forward instead.

My plan was to push this after the next KF5 release.

Note: Also see discussion here: https://mail.kde.org/pipermail/kde-frameworks-devel/2016-December/040653.html

Diff Detail

Repository
R280 Prison
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kfunk retitled this revision from to Use nullptr in all Frameworks (just diff in KIO shown here).Jan 5 2017, 11:44 PM
kfunk updated this object.
kfunk edited the test plan for this revision. (Show Details)
kfunk added a reviewer: Frameworks.
kfunk set the repository for this revision to R241 KIO.
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 5 2017, 11:44 PM
kfunk updated this object.Jan 5 2017, 11:44 PM
kfunk updated this object.Jan 6 2017, 12:06 AM
graesslin added inline comments.
src/core/job.h
50 ↗(On Diff #9785)

Question: is this change API and ABI stable?

kfunk added inline comments.Jan 6 2017, 8:07 AM
src/core/job.h
50 ↗(On Diff #9785)

Definitely ABI stable, since default arguments are not part of the function signature.

I'm not aware this could break API compat either (well, only in case the compiler consuming this code is not C++11-compliant)...

dfaure accepted this revision.Jan 6 2017, 8:58 AM
dfaure added a reviewer: dfaure.
dfaure added a subscriber: dfaure.

Change it all.

One note about "what is a public header" -- the rule is that any private header (i.e. not installed) is called _p.h
The rule isn't 100% followed though, especially in subdirs that build executables rather than libraries.

This revision is now accepted and ready to land.Jan 6 2017, 8:58 AM
kfunk updated this object.Jan 16 2017, 8:34 AM
This revision was automatically updated to reflect the committed changes.
kfunk added a comment.Jan 16 2017, 1:28 PM

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.

kfunk added a comment.Jan 16 2017, 5:20 PM
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.

kossebau reopened this revision.Jan 18 2017, 2:14 AM
kossebau added a subscriber: kossebau.

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);

or

--- a/src/kdeui/kprogressdialog.h
+++ b/src/kdeui/kprogressdialog.h
@@ -63,2 +63,2 @@ public:
-    explicit KProgressDialog(QWidget *parent = 0, const QString &caption = QString(),
-                             const QString &text = QString(), Qt::WindowFlags flags = 0);
+    explicit KProgressDialog(QWidget *parent = nullptr, const QString &caption = QString(),
+                             const QString &text = QString(), Qt::WindowFlags flags = nullptr);

or

@@ -2048 +2048 @@ void KUrlTest::testQueryItem()
-    QCOMPARE(QStringList(queryUrl.queryItems(0).keys()).join(", "),
+    QCOMPARE(QStringList(queryUrl.queryItems(nullptr).keys()).join(", "),

or

--- a/src/kdecore/kdatetimeformatter_p.h
+++ b/src/kdecore/kdatetimeformatter_p.h
@@ -49 +49 @@ public:
-                               KLocale::TimeFormatOptions timeOptions = 0,
+                               KLocale::TimeFormatOptions timeOptions = nullptr,

Just a few random hits I saw in the "9000 lines in total" file, no time to scan everything.
Sorry for the late notification, only stumbled now about similar issues in KDevelop code and assumed a similar script to be used here ;)

This revision is now accepted and ready to land.Jan 18 2017, 2:14 AM
kossebau requested changes to this revision.Jan 18 2017, 2:17 AM
kossebau added a reviewer: kossebau.

To have this somehow flagged as broken I set it now to "Request Changes". Please close again once there is a new separate diff opened.

This revision now requires changes to proceed.Jan 18 2017, 2:17 AM
kfunk added a comment.EditedJan 18 2017, 8:14 AM

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.

kossebau added a comment.EditedJan 18 2017, 2:26 PM

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);
kfunk added a comment.Jan 18 2017, 4:24 PM

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

kfunk added a comment.Jan 18 2017, 4:26 PM
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()

kossebau added a comment.EditedJan 18 2017, 4:41 PM
In D3987#78426, @kfunk 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#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.

skelly added a subscriber: skelly.Jan 18 2017, 9:26 PM
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?

kfunk added a comment.Mar 20 2017, 7:13 PM
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...

kossebau accepted this revision.Mar 23 2017, 7:55 PM
In D3987#96434, @kfunk 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.

Na, I meant, for the human reader it is semantically incorrect to pass a (null) pointer as value for a set of bitflags :) Type mismatch in human brain :)
(additional false brain alarm when one knows QFlags stores using an int, which we learnt in school to not always have bitsize of pointer type)

(Personally I count that QFlags constructor hack for dealing with literal 0 values by that pointer-type overload an example of a hack blowing up with language changes,
and now feeding nonsense stuff into the dependencies, next to bogus API dox in Qt itself, "zero must be a literal 0 value." when the default value shown right above is Q_NULLPTR, and compilers starting to complain about literal 0 being used (http://doc.qt.io/qt-5/qflags.html#QFlags-2). And my bug filed got instantly bounced, API quality seems to be not a shared value among everyone... tss...

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 ;)

Eh, I am too old to add another code-reading exception to my stable brain, but too young yet to not stand up and fight non-sense ;) Also have I had to explain too many artifacts in code to too many beginners, so I rather invest my free time into sane code ;)

So will soon make contact with that clang-tidy then, openSUSE not having it as package invites me anyway to build/run from sources...

Revision hopefully now unlocked for closing by selecting "Accept Revision" action, too bad I could not convince you to improve this yourself :)

This revision is now accepted and ready to land.Mar 23 2017, 7:55 PM
kossebau closed this revision.Mar 23 2017, 7:59 PM