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.
Details
- Reviewers
romangg - Group Reviewers
KWin - Commits
- R108:ed4a9d756aab: Clean up usage of m_client
Diff Detail
- Repository
- R108 KWin
- Branch
- master
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 14939 Build 14957: arc lint + arc unit
Besides braces this patch looks good.
useractions.cpp | ||
---|---|---|
151–154 | Use curly braces even when the body of a conditional statement contains only one line. |
useractions.cpp | ||
---|---|---|
151–154 | OK, the previous few lines don't do that, but I'm happy to follow kdelibs style. |
useractions.cpp | ||
---|---|---|
137 | 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. |
useractions.cpp | ||
---|---|---|
137 | You're right. Passing nullptr for client doesn't make sense. This if statement can be dropped. |
useractions.cpp | ||
---|---|---|
137 | 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. |
useractions.cpp | ||
---|---|---|
137 | Ok, how about:
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. |
useractions.cpp | ||
---|---|---|
137 | 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). |
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.
useractions.cpp | ||
---|---|---|
164–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? m_closeOperation = m_menu->addAction(i18n("&Close")); |
useractions.cpp | ||
---|---|---|
164–166 | Yea, you're right. Thanks for explanation. |
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.
Put the check back for now, we spin a nested event loop. Let's remove the check in an independent patch - if at all.
useractions.cpp | ||
---|---|---|
135–137 | This piece of code contradicts to Q_ASSERT above. Delete the if statement. |
useractions.cpp | ||
---|---|---|
135–137 | 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. |