Clean up usage of m_client
ClosedPublic

Authored by gladhorn on Aug 10 2019, 8:43 AM.

Details

Summary

We have a smart pointer, so be consistent in checking it.
Since there is an operator->, make the code easier to read by removing countless .data()->.
!m_client.isNull() can also be written as m_client since operator bool works.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
gladhorn created this revision.Aug 10 2019, 8:43 AM
Restricted Application added a project: KWin. · View Herald TranscriptAug 10 2019, 8:43 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
gladhorn requested review of this revision.Aug 10 2019, 8:43 AM
zzag added a subscriber: zzag.Aug 10 2019, 9:13 AM

Besides braces this patch looks good.

useractions.cpp
156–157

Use curly braces even when the body of a conditional statement contains only one line.

gladhorn updated this revision to Diff 63464.Aug 10 2019, 9:31 AM
gladhorn edited the summary of this revision. (Show Details)

Added a pile of curly brances.

gladhorn added inline comments.Aug 10 2019, 9:32 AM
useractions.cpp
156–157

OK, the previous few lines don't do that, but I'm happy to follow kdelibs style.

romangg added inline comments.
useractions.cpp
139–140

If we assert client in the beginning then cl must be non-null, right? Would make all the below checks on m_client superfluous as well though.

zzag added inline comments.Aug 10 2019, 10:35 AM
useractions.cpp
139–140

You're right. Passing nullptr for client doesn't make sense. This if statement can be dropped.

gladhorn added inline comments.Aug 10 2019, 11:25 AM
useractions.cpp
139–140

Asserts are not active in release mode. I was very conservative, but I assume all of this is single-threaded? In that case I would indeed assume that m_client is not nullptr.
On the other hand, I'd like to have my window manager crash in debug mode because of invalid assumptions and let it keep running in release builds.

romangg added inline comments.Aug 10 2019, 12:49 PM
useractions.cpp
139–140

Ok, how about:

  • Leave the assert
  • Afterwards directly do the cl.isNull() check
  • Add a comment above that you are not yet sure if the assert really holds always
  • Remove all m_client checks afterwards anyway (since we checked cl.isNull() and I don't see how the pointer can get deleted in between. It's running in a single thread.

But looking at Workspace::slotWindowOperations, the only place show is called from, it checks before that the argument is non-null. So we should not need a check.

gladhorn added inline comments.Aug 10 2019, 6:36 PM
useractions.cpp
139–140

I generally agree, but I wonder if the checks are there for a reason. And keep in mind, that this kind of nullptr check is very cheap, it's something that is so common that it's very optimized. I don't mind removing the first checks, as it was before, but I'd rather not cause a regression with the later ones. As soon as the popup is open, I'd assume nothing is guaranteed any more, since the actions in the menu could lead to clients being deleted (I'm not entirely sure).

show is called by Workspace::showWindowMenu which is called by three places without checking itself as far as I can tell (Workspace::slotWindowOperations, AbstractClient::performMouseCommand and DecoratedClientImpl::requestShowWindowMenu).

gladhorn updated this revision to Diff 63494.Aug 10 2019, 6:37 PM

Removed first nullptr check

I hope this is now making the code slightly easier to read, without changing the functionallity, except for shuffling things around a tiny bit where we discussed. It also adds a few extra checks, to play safe with the smart pointers. These nullptr checks are extremely cheap.

romangg added inline comments.Aug 11 2019, 2:32 PM
useractions.cpp
145–146

Also add braces here and below.

162–166

We set m_client = cl above and cl is guaranteed to be non-null at this point so check is not necessary.

183–185

Here check also not necessary.

I hope this is now making the code slightly easier to read, without changing the functionallity, except for shuffling things around a tiny bit where we discussed. It also adds a few extra checks, to play safe with the smart pointers. These nullptr checks are extremely cheap.

I would still like to omit them for code sanity. Checking things just to be sure pollutes the code rather easily and makes it difficult to read. If there is no parallelism in play we can say for sure if something exists or not at a certain point in time. Having decided the wrong way and then fixing it afterwards is better long-term than putting superfluous checks just to be sure.

gladhorn added inline comments.Aug 11 2019, 6:24 PM
useractions.cpp
162–166

But m_menu->exec spins an event loop. Are you sure this cannot make the client go away? What is in the user actions menu?
From what I can tell, there is even a close action. If the user opens the menu and then clicks close, shouldn't the client become nullptr? I thought that was the entire point of using a shared pointer - to track the life-time of objects after all.

m_closeOperation = m_menu->addAction(i18n("&Close"));

romangg added inline comments.Aug 11 2019, 6:30 PM
useractions.cpp
162–166

Yea, you're right. Thanks for explanation.

romangg accepted this revision.Aug 11 2019, 6:31 PM
This revision is now accepted and ready to land.Aug 11 2019, 6:31 PM
gladhorn updated this revision to Diff 63562.Aug 11 2019, 6:34 PM
gladhorn removed a reviewer: romangg.

Removed to nullptr checks - after testing manually, I cannot get it to crash with the close menu entry, so I assume this is fine indeed.

This revision now requires review to proceed.Aug 11 2019, 6:34 PM
gladhorn updated this revision to Diff 63563.Aug 11 2019, 6:35 PM

Also add curly braces.

gladhorn updated this revision to Diff 63564.Aug 11 2019, 6:41 PM

Put the check back for now, we spin a nested event loop. Let's remove the check in an independent patch - if at all.

romangg accepted this revision.Aug 11 2019, 7:04 PM
This revision is now accepted and ready to land.Aug 11 2019, 7:04 PM
This revision was automatically updated to reflect the committed changes.
zzag added inline comments.Aug 12 2019, 9:54 PM
useractions.cpp
135–139

This piece of code contradicts to Q_ASSERT above. Delete the if statement.

gladhorn added inline comments.Aug 13 2019, 6:21 AM
useractions.cpp
135–139

Remember that asserts are only in debug builds, most users will run release builds. I do agree though that this is somewhat redundant. If the assert is valid, which I think it is, then it should never trigger and we can remove the if, if the assert turns out to be wrong, then it can be removed. In either case, release builds should work and not crash, while debug builds will expose potential issues.