Use nullptr everywhere
ClosedPublic

Authored by zzag on Aug 31 2019, 8:29 PM.

Details

Summary

Because KWin is a very old project, we use three kinds of null pointer
literals: 0, NULL, and nullptr. Since C++11, it's recommended to use
nullptr keyword.

This change converts all usages of 0 and NULL literal to nullptr. Even
though it breaks git history, we need to do it in order to have consistent
code as well to ease code reviews (it's very tempting for some people to
add unrelated changes to their patches, e.g. converting NULL to nullptr).

Test Plan

Compiles.

Diff Detail

Repository
R108 KWin
Branch
use-nullptr-everywhere
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 16772
Build 16790: arc lint + arc unit
zzag created this revision.Aug 31 2019, 8:29 PM
Restricted Application added a project: KWin. · View Herald TranscriptAug 31 2019, 8:29 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Aug 31 2019, 8:29 PM
zzag updated this revision to Diff 65080.Aug 31 2019, 8:33 PM
zzag edited the summary of this revision. (Show Details)

Edit summary.

I nack this till after the upcoming Plasma 5.17 branch-off in three weeks. Then it can go in imo. Other opinions?

zzag added a comment.Aug 31 2019, 9:12 PM

I nack this till after the upcoming Plasma 5.17 branch-off in three weeks. Then it can go in imo. Other opinions?

Can you explain why?

zzag added a comment.Aug 31 2019, 9:19 PM

It's a dumb patch that converts null pointer constants to nullptr. I don't see why it can't go in 5.17.

In D23618#523182, @zzag wrote:

I nack this till after the upcoming Plasma 5.17 branch-off in three weeks. Then it can go in imo. Other opinions?

Can you explain why?

It's not a critical thing to do and I don't want work on features or fixes shortly before the release be negatively affected by a large housekeeping effort, which can go in without problems directly after branch-off as well.

zzag added a comment.Aug 31 2019, 9:24 PM

Okay, fair enough.

Thanks for your understanding. It's just a precaution. Ping me again after branch off, then I will go through the diff quickly.

davidedmundson accepted this revision.Sep 3 2019, 11:28 AM
This revision is now accepted and ready to land.Sep 3 2019, 11:28 AM

I nack this till after the upcoming Plasma 5.17 branch-off in three weeks. Then it can go in imo. Other opinions?

I assume vlad used a script, at which point you should "just" need to apply the same script to your commits before doing a rebase.

But, sure. Waiting works too

zzag added a comment.Sep 5 2019, 7:29 AM

I nack this till after the upcoming Plasma 5.17 branch-off in three weeks. Then it can go in imo. Other opinions?

I assume vlad used a script, at which point you should "just" need to apply the same script to your commits before doing a rebase.

Yeah, I ran clang-tidy with modernize-use-nullptr check. However, flags need extra care because clang-tidy likes to change 0 to nullptr, e.g. NET::States(0) -> NET::States(nullptr), etc.

zzag updated this revision to Diff 66447.Sep 19 2019, 12:27 PM

Rebase.

I have to admit in hindsight that I was maybe wrong on this one not being in 5.17. Since only having the change on master but not in the 5.17 branch could complicate porting back bug fixes. Would you guys agree to that? And since we are still at the beginning of beta phase shall we apply it also to 5.17 branch?

zzag added a comment.EditedSep 19 2019, 12:35 PM

I have to admit in hindsight that I was maybe wrong on this one not being in 5.17. Since only having the change on master but not in the 5.17 branch could complicate porting back bug fixes. Would you guys agree to that? And since we are still at the beginning of beta phase shall we apply it also to 5.17 branch?

Yes, it will make backporting patches from master to Plasma 5.17 branch difficult.

romangg accepted this revision.Sep 19 2019, 2:18 PM

Let's get it in 5.17 beta branch then as well. Tested it on X11 and Wayland and no related compile warnings.

client.cpp
1771

This is not a pure nullptr change, is it? Just for your information. If you think it's fine it's fine. See also https://doc.qt.io/qt-5/qflags.html#QFlags-2

This revision was automatically updated to reflect the committed changes.
zzag added inline comments.Sep 19 2019, 2:51 PM
client.cpp
1771

I forgot to undo that change, thanks! I'll fix default initialization of QFlags<T> in another patch.

romangg added inline comments.Sep 19 2019, 2:54 PM
client.cpp
1771

There were some more of that below but if you didn't see them before pushing not a big issue.

zzag added inline comments.Sep 19 2019, 4:08 PM
client.cpp
1771

I re-ran clang-tidy so it's not a big deal.