General Code clean-up
ClosedPublic

Authored by fabiank on Mar 29 2018, 12:45 PM.

Details

Reviewers
aacid
Group Reviewers
KDE Games
Commits
R410:b28294856639: General Code clean-up
Summary

simplify if condition

always initialize variable

directly initialize variables

remove dead code

reduce variable scope, remove dead comments

remove declared, but unimplemented function remove unused global variable

removed unused member, removed member which was only ever read, never written

remove unnecessary forward declaration

remove declaration of struct that is defined below

remove redundant cast convert malloc + memset to calloc

use bool literals (clang-tidy)

use nullptr instead of 0 (clang-tidy)

Test Plan

Manual regression testing: code still compiles, games still run, and solver can
win the game

Diff Detail

Repository
R410 KPatience
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
fabiank created this revision.Mar 29 2018, 12:45 PM
Restricted Application added a reviewer: KDE Games. · View Herald TranscriptMar 29 2018, 12:45 PM
Restricted Application added a subscriber: KDE Games. · View Herald Transcript
fabiank requested review of this revision.Mar 29 2018, 12:45 PM

@aacid : You were joking that I've become the de-facto maintainer of the solver code by contributing to it, so I thought I might as well improve the general code hygiene. This patch set mainly removes provably dead code, and applies a few clang-tidy modernizations. The remainder is a bit of code simplification (reducing variable scope, malloc + memset -> calloc). I couldn't figure out how to get arc to create a series of smaller review requests (one per commit, history.immutable = true doesn't help in that regard), and hope you don't mind the clustered review request.

Looks good to me. But wait a few more days in case others want to review as well.

gameselectionscene.cpp
149

Just a hint: If you are using "nullptr" (which is good), you could also switch "Q_DECL_OVERRIDE" with "override".

libkcardgame/kcardscene.cpp
85

The C++ Core Guidelines recommend leaving the comparison in this case, so Q_ASSERT( !object ); would be enough.

fabiank added inline comments.Mar 29 2018, 1:10 PM
gameselectionscene.cpp
149

I was actually wondering about that one: Do we still want to support older compilers? In that case, we should probably go for Q_NULLPTR. If not, I'll also switch Q_DECL_OVERRIDE to override.

fabiank updated this revision to Diff 30846.Mar 29 2018, 1:34 PM
  • simplify check for nullptr in assert
fabiank marked an inline comment as done.Mar 29 2018, 1:35 PM
aacid added a comment.Mar 30 2018, 7:56 AM

Quick answer to one of the questions,i'll have a more throguh look later/tomorrow

gameselectionscene.cpp
149

We can support what we want, but at this point i think KDE Frameworks just uses nullptr so i don't see why we would want to be more conservative

aacid accepted this revision.Apr 1 2018, 6:16 PM
This revision is now accepted and ready to land.Apr 1 2018, 6:16 PM
This revision was automatically updated to reflect the committed changes.
aacid added a comment.Apr 1 2018, 6:19 PM

I'd say you can use override instead of Q_DECL_OVERRIDE if you want

I'd say you can use override instead of Q_DECL_OVERRIDE if you want

Okay, that (and adding a few overrides where it makes sense) can be done in a further patch. By the way, I have commit rights, so you don't have to do it on my behalf (not that I mind either way).