----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121831/#review74463 -----------------------------------------------------------
I think it's a good idea to hide the impl behind a d-pointer. It's yet unclear to me, whether the additional setter/getter calls are really a performance issue. If required, we could save storing local variables by adding reference-getters, see comments below. But I'd only do this if we have input that this is really necessary. Comments from the KSysGuard authors would be very much appreciated :-) processcore/process.h <https://git.reviewboard.kde.org/r/121831/#comment51643> Is virtual needed here? processcore/process.h <https://git.reviewboard.kde.org/r/121831/#comment51644> You can do it like this. Q_DECLARE_PRIVATE does something along these lines: #define Q_DECLARE_PRIVATE(Class) \ inline Class##Private* d_func() { return reinterpret_cast<Class##Private *>(qGetPtrHelper(d_ptr)); } \ inline const Class##Private* d_func() const { \ return reinterpret_cast<const Class##Private *>(qGetPtrHelper(d_ptr)); } \ friend class Class##Private; Furtheri, Q_D looks like this: #define Q_D(Class) Class##Private * const d = d_func() So you can either use: Q_D(Process); d->... or d_ptr->... or d_func()->... Personally, I prefer declaring a d-pointer directly, since it does not require any macro an das such you can directly see what happens, without any Q_D or d_func() magic. So in essence: the way you do it is correct, it's just a matter of taste. Right now, you always need the extra line "Q_D(const Process);" in the getters. You could save these by either writing d_ptr->, or by going the pure d-route. Would be a bit less code (which imho is good), but as said, matter of taste. Please remove the comment :-) processcore/process.cpp <https://git.reviewboard.kde.org/r/121831/#comment51646> Can you write: KSysGuard::Process::Process() : d_ptr(new ProcessPrivate()) { clear(); } Although the curly brace often is in the same line in this class, the kde coding style is more the above. processcore/process.cpp <https://git.reviewboard.kde.org/r/121831/#comment51647> Same here. processcore/process.cpp <https://git.reviewboard.kde.org/r/121831/#comment51648> Yes, needed. processcore/process.cpp <https://git.reviewboard.kde.org/r/121831/#comment51649> This line is wrong: (d->tracerpid == d->tracerpid) is always true. processcore/processes_linux_p.cpp <https://git.reviewboard.kde.org/r/121831/#comment51650> In theorey, if we wanted to avoid the local varialbes here, we could add reference-getters, e.g.: qlonglong & process->ruid(); These reference getters could be used as parameters to store the data directly in the desired varialbles, which would equal the current behavior. Not sure it's worth it, would be cool to have input from true KSysGuard developers here. processcore/processes_linux_p.cpp <https://git.reviewboard.kde.org/r/121831/#comment51651> Don't you change behavior here? Before, we just wrote into the varialbes. Now, we use the setters, which also sets 'changes |= Process::Gids;' Is that maybe an issue? I myself don't know the code well enough to see this here. - Dominik Haumann On Jan. 19, 2015, 9:37 nachm., Gregor Mi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/121831/ > ----------------------------------------------------------- > > (Updated Jan. 19, 2015, 9:37 nachm.) > > > Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John > Tapsell. > > > Repository: libksysguard > > > Description > ------- > > In process.h there are several public fields. I propose to encapsulate the > private fields with getter methods. I implemented it exemplary for the fields > 'login', 'uid' and 'euid'. > > > (In a separate commit I would add the .reviewboardrc file) > > What is the current policy on using small C++ macros as done in this RR? Use > it (code is more compact and readable) or don't use it (better for debugging)? > > > Diffs > ----- > > .reviewboardrc PRE-CREATION > CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636 > processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a > processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab > processcore/processes.cpp 6c0effc903b7792a078e68d2ac6f7ac29dbbcc31 > processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601 > processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5 > processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 > processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889 > processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420 > processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 > > Diff: https://git.reviewboard.kde.org/r/121831/diff/ > > > Testing > ------- > > Compiles and runs. Data is still shown. No error. > > > Thanks, > > Gregor Mi > >
