GNOME Bugzilla – Bug 773016
PATCH: Cgroup reform
Last modified: 2016-10-28 13:48:12 UTC
Created attachment 337781 [details] [review] Cgroup reform patch This patch introduces new cgroup detection and parsing code. Key highlights: - Elimination of manual allocations, usage of glibmm structures. - No gotos. - Code that is actually readable.
Thank you for your contribution. It took me a bit longer to respond as I saw a couple of errors on the log after applying the patch, but they weren't caused by your patch, but by GTK+ changes. After fixing those, the patch works, looks fine, so I am committing it to master, and should be available with the next development release.
I'm just curious why for the unary NOT operator, the patch sometimes uses ! and sometimes uses the word "not" - it seems inconsistent, and this patch is actually the first time I've seen the latter used in production. More to the point, aren't those operators actually defined as macros in a different header, which is never actually #included? That seems like a build failure waiting to happen. Not to mention people wondering what those operators are, as I think recognition is much less common than the symbolic forms.
@djb: thank you for your comment. Indeed, I also prefer the ! form of the unary NOT operator. However, system monitor uses both forms since quite a long time (in production :) ), so build failure should not happen, as it would have happened to me when building the code with the patch, or building any other part of system monitor
"not" and "and" are alternative spellings for "!" and "==" operators respoectively. They are defined in the standard as well. While originally created for lacking keyboard layouts, I see them used in modern C++ projects because of clarity that they provide. Basically, your code inches from "code" towards plain English (when you can literally write "if not (A and B) then { do-something }").
Hello Robert, I was talking with email with Artem. I'm not satisified with the performance of this patch and it also contains a bug (that was also may be there in the previous code about name/category with a ':' on it. I will commit next week the improved and fixed version (I'm very busy this weekend).
Created attachment 338675 [details] [review] Faster, better parsing and nicer output What do you think about this ?
I'm clueless about the fine details of what the code is doing.., but this new one is certainly infinitely more readable than the previous. All the concatenated stuff like "if (condition) { then; }" was especially obfuscating IMHO. If the rationale is that above: > your code inches from "code" towards plain English > (when you can literally write "if not (A and B) then { do-something }"). then I would not count that as a benefit, since C++ is not and will never be English, and it needs its own formatting, indentation, etc to accommodate this and make it easier to read.
And you are absolutely right, I wasn't even sure about this part.
Artem, i've pushed my batch of cgroups update. Please have a look and ping me if you have ideas for improvement. I'm satisfied with the current status because it enabled us to fix a bug (not all cgroups were parsed) and improved the readability of the cgroup column.