adridg added inline comments.
INLINE COMMENTS
> adridg wrote in kprocesslist_unix_procstat.cpp:38
> I absolutely have Opinions on C++ style that apply here, so I'm going to talk
> to Tobias elsewhere.
In order to simplify cleanup, I'd use a RAII class as well:
/** @brief Wrapper around procstat_open_sysctl()
*
* Hangs on to a procstat instance for its friend classes, and closes
* it on destruction. Cast to bool to check for success.
*/
struct ProcStat
{
ProcStat()
{
pstat = procstat_open_sysctl();
}
~ProcStat()
{
procstat_close(pstat);
}
operator bool() const
{
return pstat;
}
struct procstat *pstat;
};
Lines 37-42 become `ProcStat p; if (!p) return rc;`, but more importantly you
can drop cleanup from other return paths since the destructor handles it.
> kprocesslist_unix_procstat.cpp:46
> + struct kinfo_proc *procs;
> + procs = procstat_getprocs(pstat, KERN_PROC_PROC, 0, &proc_count);
> +
Similarly, you could
struct ProcStatProcesses
{
public:
ProcStatProcesses(ProcStat& pstat) : parent(pstat)
{
procs = procstat_getprocs(parent.pstat, KERN_PROC_PROC, 0,
&proc_count);
}
~ProcStatProcesses()
{
if (procs)
{
procstat_freeprocs(parent.pstat, procs);
}
}
operator bool() const
{
return procs && proc_count > 0;
}
ProcStat& parent;
unsigned int proc_count;
struct kinfo_proc *procs;
}
I'd probably also write an iterator for it, returning KProcessInfo from
`operator*()`, so that the magic is hidden away in small classes and the
overall algorithm becomes a very short bit of text. You might call that
over-engineered, since there's nothing wrong with the code as-written.
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D21146
To: tcberner, #freebsd, adridg, davidedmundson
Cc: pino, apol, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns
