I recently stumbled upon this excellent 2014 article about patch review by Sage Sharp: The Gentle Art of Patch Review.

I highly recommend reading the whole article, but my main takeaway is that when reviewing code, one should follow three phases:

1. Answer the question: "Good or bad idea?"
2. "Is this architecturally sound?": are the changes done in a way that makes sense given the existing project architecture?
3. only then, can you go all in on nitpicking on naming, spelling mistakes, or ask for the commit history to be cleaned.

I do quite a lot of reviews at work. Sage article made me realize I am often guilty of jumping directly to #3. I have been reflecting on why that happens, and I concluded that it's because it's the path of least resistance.

When I receive a 10,000 line patchset, with 23 commits stepping on each other and no description, the temptation is high to skip phase 1 and 2 and instead say to myself: "I am just going to review the whole patch one line at a time, suggesting new names and micro-optimizations so that it does not look like I clicked on Approve while looking elsewhere".

Since jumping directly to phase 3 is the path of least resistance, when preparing code for review, you should make what you can to reduce the resistance of phase 1 and 2.

Phase 1: Is this a good or bad idea?

For this phase, the place to make review easier is in the description of your patchset: that is the pull request description on GitHub or the body of the mail presenting the patchset in a mail-based workflow.

It does not have to be extra long, just explain in one or two sentences the problem fixed by the patchset, or the new feature it adds... If the changes are significant, then hopefully you discussed them before diving full in: maybe there is an opened issue in the bug tracker, or it was discussed by chat or on a mailing-list. If so, add a link to it to your description.

Phase 2: Are the changes architecturally sound?

For this phase, there are 3 places to make review easier: patchset description, high-level comments and commit history.

Patchset description

The patchset description should not paraphrase the patchset, but it should give a high-level overview of the changes.

Let's say as part of your patchset, you moved class Foo from file A to file B. If a reviewer goes directly through the changes (as in, jumps directly to Phase 3), they will have to infer that given that class Foo was removed from A, and a very similar (but maybe not identical!) class Foo was added to B, then it means the patchset moved class Foo from A to B. By listing this move in your description, not only do you save the reviewer the pain of having to infer the move, but you also get the chance to explain why you moved this class.

This can be hard. Maybe you do not like writing so this is chore. Well, being able to write prose is a very useful skill, even for the developers! Stepping out of your comfort zone is difficult, but the more you practice, the better your writing will be, the less it will become a shore.

Another reason this can be hard is because you need to write this in English and you think your English is not good enough. As a friend once told me: "The language of Open-Source is broken English", so don't be afraid of this. A broken English description is much more helpful than no description at all, and if someone criticizes your English, well... shame on them! You did the work! And again, practice will improve your English writing too.

High-level comments

Some coding guidelines require every function, every constant, every class, every module to have documentation. I think those are misguided and often lead to Captain Obvious comments like that:

def activate_rear_engine():
    """Activate the rear engine"""
    ...

Thank you, Captain Obvious

On the other hand, having a comment attached to a new class or a module is very helpful, especially if it explains how the class works with other classes. Again, keep it high-level.

Commit history

Ideally changes should be small, but it's not always possible. If the changes are large, chances are they can be split in multiple smaller changes. It is a lot easier to review a large patchset if one can go through it commit by commit.

Except... this is a double-edged sword! It is only easier to review if each commit makes sense in isolation and if the set of commits "tell a story". If a commit adds a method to a class and the next commit renames or removes it with a "Oups, that was not a good idea" commit message, then it actually takes longer and is more painful to review commits individually than just reviewing the final state.

This is easier said than done, though: when you are working on your changes, it's common to hit road blocks, change directions, or make unrelated changes. The commit story usually won't be straight and easy to read on your first try.

You know what? That's fine! Just like the first draft of a novel writer is not perfect, so is your commit story. And just like the novel writer, you can edit your story to make it easier to read. Assuming you work with a VCS which lets you rewrite history like Git, once you are done with your first draft, it's time to warm up git rebase!

Again, this takes practice to get right, but here are a few tips to get started:

• Splitting commits is more complicated than joining them, so it's simpler to create smaller commits and join them later.
• You can create partial commits using git add -p or git gui.
• Before starting a large history rewriting session, create a quick backup branch with git branch backup. This serves two purposes:
  1. If things go wrong, it's easy to revert to a known good state using git reset --hard backup (you can also use git reflog to do that, but I find this approach simpler).
  2. When you are done with your rewrite, you can run git diff backup to check if the end result is similar enough.
• Maybe some of the changes are unrelated? If it's possible, move them to a separate branch and create a separate patch review for them. The smaller the changes, the easier it is to review.

Addressing review issues

This part depends on the policy of the project. Some projects prefer if you force-push the fixed patchset as you address issues spotted during review, so that the commit history always looks like what will be committed. Other projects have a no-force-push policy, they will ask you to push fixup commits. This often happens on forges which lack the ability to compare patch-sets (Unfortunately, GitHub is one of them, GitLab is much better in that regard).

If you get asked to do fixup commits, please make sure the history is squashed into a readable and coherent history before the patchset is merged. This greatly simplifies the lives of future developers if they have to revisit the past.

Wrapping up

To wrap up, to make the life of your reviewer easier (and thus make the review faster and/or more insightful):

• Provide a high-level description of your changes, refer any prior discussion about it.
• Without going all-in and adding comments everywhere, do provide high-level comments for new classes or modules.
• Try to keep your commit history easy to read.

You may also find out during this preparation process that some of your changes are not optimal, or a bug slipped in. When this happens, take the time to fix these issues before posting your patchset for review. In a sense, this process makes you the first reviewer of your code. And the cleaner your patchset, the faster the review!

I'm excited to announce the release of version 0.8 of libQuotient, the Qt library for building software using Matrix.

This release brings to you

• Generated documentation, browsable at quotient-im.github.io/libQuotient/
• Coinstallability of Qt5 and Qt6 builds
• Various stability improvements
• Updated Matrix APIs

More Information & Tarballs

Available at the release page on GitHub :)

But wait, there's more!

End of next week, Akademy, KDE's annual conference is starting in Thessaloniki, Greece and online. Alexey, Carl, and I will be there talking about our work on and with Matrix and ActivityPub. The talk will be live-streamed, so don't miss it!

Getting involved

There's always more things to do in libQuotient. Come talk to us in #quotient:matrix.org.

It’s time for a new Kirigami Addons release. This new release contains the work of Joshua Goins, Laurent Montel, Thiago Sueto, Volker Krause, Shubham Arora, James Graham, Rishi Kumar and myself. Thanks for contributing!

New features

MobileForm.FormGridContainer

FormGridContainer makes it possible to make the information exposed with MobileForm more compact by putting multiple small cards in the same row. The number of columns by default is 3 on desktop unless the total number of cards makes it more sensible only to use 2 columns (e.g when there are only 2 or 4 elements).

On mobile the number of columns is reduced to only 2.

This new component was initially made by Rishi Kumar for the info grid that Tokodon, with some further improvement from myself. Here is how this looks with Tokodon.

And the current API:

import org.kde.kirigamiaddons.labs.mobileform 0.1 as MobileForm

MobileForm.FormGridContainer {
 infoCards: [
 MobileForm.FormGridContainer.InfoCard {
 title: "42"
 subtitle: i18nc("@info:Number of Posts", "Posts")
 },
 MobileForm.FormGridContainer.InfoCard {
 title: "42"
 },
 MobileForm.FormGridContainer.InfoCard {
 // Clickable card
 title: "Details"
 action: Kirigami.Action {
 onClicked: pageStack.push("Details.qml")
 }
 }
 ]
}

MobileForm.AboutPage now can contains extra content

Applications can now extend the AboutPage to add extra content to it. This was driven by the need of Itinerary which need to expose the license information about the open data it uses.

Validor support in MobileForm.FormTextFieldDelegate

Application can now add a Validor to their textfield. This was driven by the need of Keysmith rewrite to use MobileForm.

Important bugfixes

The BasicTreeItem component now uses correct spacing between items. This was caused by a regression when adding right to left support to the KirigamiAddons TreeView.

The native Android date/time pickers now works correctly even if multiple instance of it are loaded at the same time.

Shubham and James fixed various bugs with the maximized image and video component which is displayed in NeoChat and Tokodon when viewing media files.

It is the first day of July… time for me to have a large chunk of vacations it didn’t really happen for a while and I feel I need it.

During those vacations I’ll try to stay away from the news as much as possible. I guess the fashionable way to put it is: I will have a three weeks “news detox”. 😊

So, don’t expect web reviews during that time.

My plan is of course to read books, visit places and spend as much quality time with family as possible. And still… it won’t be all play. There are a few software and writing projects I’ve been postponing for far too long and I’ll try to at least get them to progress a bit.

I’ll also take a few days away from my family to attend Akademy!

I’m of course excited to reconnect and chat with everyone. See you there!

Last but not least, I won’t stay at Akademy for long in order to run back to my family, but… I’ll be holding an online workshop about the KDE Stack during the training day.

Since it’ll be in the online room it should be easy for anyone interested in the topic to attend. 😉

See you there!

Let’s go for my web review for the week 2023-26.

Criminalization of encryption : the 8 december case – La Quadrature du Net

Tags: tech, france, law, foss, surveillance, criticism, cryptography

A reminder of what’s going on in France… and it’s bleak. Lots of things can turn you into a suspect at this point.

You encrypt your communications or data? You try to avoid surveillance from the GAFAM? You use Tor or a VPN?

If anything bad happens around you, then you’ll turn into a suspect, this is due to the “clandestine behavior” you entertain… so for sure you are part of some “conspiracy”.

This is very worrying.

https://www.laquadrature.net/en/2023/06/05/criminalization-of-encryption-the-8-december-case/

How to Kill a Decentralised Network (such as the Fediverse)

Tags: tech, gafam, facebook, fediverse, xmpp, standard, community

Facebook getting interested in the fediverse indeed looks like XMPP or OOXML all over again. Beware of those old tactics, they are very efficient against communities.

https://ploum.net/2023-06-23-how-to-kill-decentralised-networks.html

Artificial Inteligence: why I’ll not hashtag my art #HumanArt, #HumanMade or #NoAI - David Revoy

Tags: tech, ai, machine-learning, art

Excellent piece from an excellent artist. I really thought this through and I think he’s going in the right direction.

https://www.davidrevoy.com/article977/artificial-inteligence-why-i-ll-not-hashtag-my-art-humanart-humanmade-or-noai

Google doesn’t want its employees using Bard code • The Register

Tags: tech, ai, machine-learning, gpt, google

It smells a bit like hypocrisy isn’t it? On one hand they claim it can make developers more productive on the other they think they shouldn’t use it.

https://www.theregister.com/2023/06/19/even_google_warns_its_own/

About GitHub’s use of your data - GitHub Docs

Tags: tech, github, copyright, copilot

Basically the wording allows them to feed whatever system they want with your code… even in private repositories.

https://docs.github.com/en/get-started/privacy-on-github/about-githubs-use-of-your-data

A Cookbook of Self-Supervised Learning

Tags: tech, ai, machine-learning, research

Very comprehensive (didn’t read it all yet) guide about self-supervised learning. It’ll likely become good reference material.

https://arxiv.org/abs/2304.12210

Time is not a synchronization primitive - Xe Iaso

Tags: tech, tests

Especially important in the context of tests indeed. As soon as you depend on some form of elapsed time you get into the territory of flaky tests.

https://xeiaso.net/blog/nosleep

Tips for concurrent programming

Tags: tech, programming, distributed

Nice set of advises when dealing with concurrency. Don’t fall into some of the anti-patterns which are pointed out.

https://catern.com/concur.html

Programming Languages Going Above and Beyond

Tags: tech, programming, compiler, safety

This is a neat example of what programming languages could check at compile time. This clearly brings way more safety when you get such contract validation at build time.

https://whileydave.com/2023/06/27/programming-languages-going-above-and-beyond/

Making C++ Memory-Safe Without Borrow Checking, Reference Counting, or Tracing Garbage Collection

Tags: tech, static-analyzer, c++, memory, safety

Long but fascinating article on a blend of guidelines which could be statically checked to enforce a memory-safe subset of C++.

https://verdagon.dev/blog/vale-memory-safe-cpp

Rust fact vs. fiction: 5 Insights from Google’s Rust journey in 2022 | Google Open Source Blog

Tags: tech, programming, rust

Interesting to see what gets confirmed (slow compiler, nice compiler error messages, code quality) or debunked (steep learning curve, interoperability).

https://opensource.googleblog.com/2023/06/rust-fact-vs-fiction-5-insights-from-googles-rust-journey-2022.html

Gatherers

Tags: tech, java

This would be an interesting extension to the Java Stream API. Maybe one day it’ll make its way to the standard…

https://cr.openjdk.org/~vklang/Gatherers.html

You should break the Law of Demeter

Tags: tech, architecture, design, programming, craftsmanship

I always felt uneasy around this “law” as well. It’s a good deconstruction of it and proposes proper alternatives. It’s all about dependencies really.

https://www.tedinski.com/2018/12/18/the-law-of-demeter.html

XML is the future - by Nobody has time for Python

Tags: tech, hype, complexity, technical-debt

Good piece about the hype cycles our industry constantly fall into. So much undue complexity for nothing in some projects… and then we’ll complain about technical debt. It would have been much easier to pick the right tool instead of wanting to use whatever new got released by some big tech company.

https://www.bitecode.dev/p/hype-cycles

Twilight Of The Programmers

Tags: tech, programming, logic, engineering, craftsmanship

Interesting way to look at our profession… I wonder if this is the core reason of why we have a hard time to turn into a proper engineering discipline, is it even possible at all then?

https://danielbmarkham.com/twilight-of-the-programmers/

To Build a Top Performing Team, Ask for 85% Effort

Tags: management, productivity, burnout

Good tips on how to create some slack and prevent burnout.

https://hbr.org/2023/06/to-build-a-top-performing-team-ask-for-85-effort

Bye for now!

We’re about halfway through year! This update is a bit smaller than usual, and more focused on applications than Plasma. This isn’t for lack of time or trying, but I tried to deliberately clear out my backlog. That goal didn’t really pan out, but I’ll be trying again next month.

Craft

The Android build for Tokodon (and consequently it’s CI) was broken because I replaced the video player with libmpv, so I spent a good chunk of this month making sure it’s working again. This was a little difficult to fix, but I feel much more confident with Craft now.

If you’re not familiar with Craft, it’s a meta-build system created by KDE. Craft and it’s blueprints are written in Python. Blueprints describe how to build the application or library, and has easy utilities for working with existing build systems (AutoTools, CMake, Meson, etc). It may be a little daunting, but these blueprints easy to write and maintain. More importantly, Craft enables easy cross-compilation since it contains blueprints for the underlying libraries for KDE applications: OpenSSL, zlib, and so on.

Tokodon is (to my knowledge) the first KDE application to use libmpv on Android, so I needed to break a lot of new ground to make this happen. What’s exciting though is that any KDE application that uses libmpv (PlasmaTube?) doesn’t have to jump through hoops to make it build on Android.

Sorry, this section will be mostly text because I can’t exactly visualize build files!

zlib

zlib is already included in the Android NDK, but for some reason they don’t ship pkgconfig files for it (or anything, really). For example, Freetype declares a dependency on zlib in it’s pkgconfig, and then pkgconfig (the program) starts complain that it can’t “find zlib” although the library itself exists. That’s because pkgconfig is searching for zlib’s config, and doesn’t care if the .so or .a file exists on disk anyway.

For now, I propose enabling zlib on Android anyway. It’s odd that we have to build it again, but I don’t see an easier solution right now.

kirigami-addons

This is a really simple fix, QtMultimedia was missing as a dependency for this library. The new fullscreen image viewer uses QtMultimedia under the hood to display video, but it wasn’t declared in the blueprint yet.

See the merge request adding it as a dependency.

libarchive

The versions of libarchive currently in Craft had an Android-breaking header inclusion bug, preventing compilation if you use any of it’s headers.

Now it’s bumped to 3.6.2 (the latest version as of writing), see the merge request. Support for the last version is also being dropped, let’s see if that breaks anything!

ffmpeg

Tokodon uses libmpv to display videos from Mastodon and other services, which are served over HTTPS. libmpv uses ffmpeg to fetch video data, but the Craft blueprint for ffmpeg didn’t enable the TLS backend.

See the merge request which enables the OpenSSL backend for all platforms, which could benefit other KDE applications!

fribidi and mpv

When deploying to Android, we use a tool called androiddeplyqt to package the libraries required by an application. One of the checks it deploys is checking the version of the .so before putting it inside of the APK. If your application links to, say libmpv.so.1 then androiddeployqt will stop in it’s tracks. Fribidi and mpv felt the need to set versions, without a way to tell them to stop.

See this merge request which now builds these two libraries statically completely side-stepping the issue. I think this is an okay solution for now, as a shared library doesn’t make much of a difference on Android. If someone knows a better way to force them to stop putting out versioned shared libraries, let me know.

Meson

Some dependencies of mpv (Such as fribidi) used Meson as it’s only build system, which didn’t have cross compilation support in Craft yet. Meson is nice and all, but it’s cross compilation system is really obtuse, as I need to write a file! This is fed into the meson command, but I don’t understand why we can’t pass these as arguments or environment variables.

See the merge request for enabling cross compilation for Meson projects under Craft. There’s still a bit of work left to do for me, like figuring out how to handle switching between toolchains.

PlasmaTube

I’ve been using PlasmaTube more lately, so that means more and more fixes to it! First, non-video results are no longer displayed. That’s these blank little fellows in the video grid:

The two player modes (minimized and maximized) are now stored as a binary flag, instead of guessing it. In simpler terms this means that when you maximize the window, it doesn’t enter a weird state where half of the player is open.

I improved the video loading experience a bit. Now when you explicitly stop a video it doesn’t momentarily reappear too soon when you click on another one. This doesn’t affect clicking between videos while it’s still playing or paused though.

The videos on a channel page load now! It’s not categorized or anything yet, but it’s a good start.

And of course, add an “About KDE” page to the settings!

Tokodon

I completed some important tasks for Tokodon this week! It may seem boring, but this includes important groundwork (including the Android stuff above) so we can focus on implementing more useful features.

Better authentication

I’m starting to remove the manual authentication code step which means it’s now even easier to login to Tokodon. Once you complete the login flow in your browser, it will return you to Tokodon magically. How to get this working in a Qt/KDE application is a mystery apparently (and on Android) so that might be future blog material.

I have some local changes enabling support for this on Android, and I’m hopeful this will appear in the next release. As a bonus, it lets you log into Pixelfed! For some reason, they don’t support copying the authcode and only support URI callbacks to return to the application. Once this lands, Tokodon can work as a Pixelfed client!

Notification improvements

Now the notifications coming from Tokodon is a little bit better. I merged a change that fixes the notification identity so it’s coming from who actually did the action. On top of that, the avatar is rounded which matches what we do inside of Tokodon as well:

Android improvements

I merged a bunch of small fixes which makes the Android experience a little bit better.

• The settings page no longer crash because Sonnet was erroneously included on Android.
• There was a bunch of missing icons on Android, now they are included!
• When entering the instance URL during the login flow, the keyboard doesn’t try to uppercase the first letter.

Config improvements

I landed the much needed config overhaul which fixes two major pain points: configuration location and security. We used KConfig but only for the main configuration options, not for the account information. This meant there was this odd ~/.config/KDE/tokodon.conf and contained secret information to boot. Now everything is handled by KConfig, and the volatile account information is stored separately from the main configuration options in ~/.local/share/Tokodon/tokodonstaterc (possibly moving to ~/.local/state in KDE Frameworks 6).

The account token and client secrets are now stored in your system keychain (like KWallet or GNOME Secrets)! When you start Tokodon 23.08, it will automatically migrate your config files, so there’s nothing you need to do.

Sharing

Now you can share posts, images, and profiles directly from Tokodon! I think this is useful for quickly sharing between devices.

I’m also working on sharing through Tokodon! It still needs some work. My current plan is to have it open the composer if already running, or open in a standalone composer window if it isn’t.

See you next month!

Hello world,

This is my fourth blog post and a continuation to my previous blog posts for Google Summer of Code 2023 under KDE.

In this blog, I will be sharing my experiences during the 3rd and 4th weeks of GSoC'23.

Week 3 | Finalizing Account Moderation tool

In my previous blog post, I mentioned that I had worked on implementing the initial page of the Account Moderation Tool in the first two weeks. This week, I began implementing the MainAccountToolPage.qml which serves as the main page of the tool where moderators can view admin-level details and take actions against an account.

I started by parsing the API response JSON and implementing all the necessary models and methods in the cpp backend. The most interesting part while implementing the backend code was determining the API call for POST request. Initially, I was considering writing separate methods for each endpoint but after going through the source code of Tokodon, I noticed how cleverly Tokodon implements hash maps to handle different endpoints for a POST request. So I went with a similar implementation for my tool.

Next was implementing the QML front-end. As I am relatively new to writing QML and working with Kirigami frameworks, this part was rather more challenging. Fortunately, I have been a plasma user for a long time, so whenever I got stuck, I would refer to other KDE application’s source code or ask my mentor to help me out.

Finally, after lots of refactoring and code reviews, the maintainers approved my MR, and it got successfully merged into the master branch 😮‍💨.

Images of implemented account moderation tool.

Week 4 | Adding the initial page of the Report Moderation tool

In the fourth week, I started with the implementation of the Report Moderation tool into the codebase. As I had already implemented the Account Moderation tool, I expected things to be similar this time. I started with implementing cpp backend first, the only difference this time was using the Post class to parse the reported status. Using Post class to parse the reported status was a bit tricky to figure out as I had initially thought of writing individual methods for each parameter in ReportInfo class which would have been inefficient.

On the QML side, things didn’t go as smoothly this time. I faced many binding and polish loops while laying out the content, which were very tricky to solve. The QML error logs didn’t point to any specific line number or code block so I had to fix them by isolating individual blocks of code and debugging them individually.

By the end of the 4th week, I was able to implement the initial page of The Report Moderation tool. The MR is still under review and you can track it here

Image of the implemented initial page of the Report Moderation Tool.

I will be writing regular blog posts on my website. You can read my previous blog-posts and follow my progress here

And what about…

While talking about the build time improvements seen by avoiding the use of Qt module header includes Volker Krause wondered: in chat

regarding the compile time improvements, I have the suspicion that included moc files would help with incremental build times, possibly even quite noticeably (compiling the combined automoc file can be quite expensive), but no idea how that impacts clean builds

And while he was occupied with other things, this suspicion caught my interest and curiousity, so I found some slots to give it some closer look and also learn some more.

After all, people including myself had removed quite some explicit moc includes by the years, also in KDE projects, enjoying existing automoc magic for less manual code. Just that in the mean time, as soon noticed, Qt developers had stepped up efforts to add them for the Qt libraries were missing, surely for reasons.

Back to the basics…

Let’s take a simple example of two independent QObject sub-classes Foo and Bar, with own header and source files:

foo.h: class Foo : public QObject { Q_OBJECT /* ... */ };

bar.h: class Bar : public QObject { Q_OBJECT /* ... */ };

foo.cpp: #include "foo.h" /* non-inline Foo method definitions */

bar.cpp: #include "bar.h" /* non-inline Bar method definitions */

CMake’s automoc will detect the respective Q_OBJECT macro usages and generate build system rules to have the moc tool create respective files moc_foo.cpp and moc_bar.cpp, which contains the code complementing the macro (e.g. for the class meta object).

CMake then, if no source files include those generated moc files, will have added rules to generate for each library or executable target a central file mocs_compilation.cpp which includes those:

// This file is autogenerated. Changes will be overwritten.
#include "<SOURCE_DIR_CHECKSUM>/moc_foo.cpp"
#include "<SOURCE_DIR_CHECKSUM>/moc_bar.cpp"

This results in a single compilation unit with all the moc code. It is faster to build compared to compiling all moc files in separate ones. Note the “all” here, as all moc code only needs to be build together in full project (re)builds.

Incremental build, wants to handle minimal size of sources

When working on a codebase, one usually does incremental builds, so only rebuilding those artifacts that depend on sources changed. That gives quick edit-build-test cycles, helping to keep concentration stable (when no office-chair sword duel tournaments are on-going anyway).

So for the example above when the header foo.h is edited, in an incremental build…

1. the file foo.cpp is recompiled as it includes this header…
2. next moc_foo.cpp to be regenerated from the header and then…
3. mocs_compilation.cpp to be recompiled, given it includes moc_foo.cpp.

Just, as mocs_compilation.cpp does not only include moc_foo.cpp, but also moc_bar.cpp, this means also the code from moc_bar.cpp is recompiled here, even if does not depend on foo.h.

So the optimization of having a single compilation unit for all moc files for headers, done for full builds, results in unneeded extra work for incremental builds. Which gets worse with any additional header that needs a moc file, which then also is included in mocs_compilation.cpp. And that is the problem Volker talked about.

Impact of mocs_compilation.cpp builds

On the author’s system (i5-2520M CPU @ 2.5 GHz, with SSD) some measurements were done by calling touch on a mocs_compilation.cpp file (touch foo_autogen/mocs_compilation.cpp), then asking the build system to update the respective object file and measuring that with the tool time (time make foo_autogen/mocs_compilation.cpp.o).

To have some reference, first a single moc file of a most simple QObject subclass was looked at, where times averaged around 1.6 s. Then random mocs_compilation.cpp found in the local build dirs of random projects were checked, with times measured in the range of 5 s to 14 s.

Multiple seconds spent on mocs_compilation.cpp, again and again, those can make a difference in the experience with incremental builds, where the other updates might take even less time.

Impact of moc include on single source file builds

Trying to measure the cost which including a moc file adds to (re)compiling a single source file, again the tool time was used, with the compiler command as taken from the build system to generate an object file.

A few rounds of measurement only delivered average differences that were one or two magnitudes smaller than the variance seen in the times taken, so the cost considered unnoticeable. A guess is that the compiler for the moc generated code added can reuse all the work already done for the other code in the including source file, and the moc generated code itself not that complicated relatively.

This is in comparison to the noticeable time it needs to build mocs_compilation.cpp, as described above.

Impact of moc includes on full builds, by examples

An answer to “no idea how that impacts clean builds” might be hard to derive in theory. The effort it takes to build the moc generated code separately in mocs_compilation.cpp versus the sum of the additional efforts it takes to build each moc generated code as part of source files depends on the circumstances of the sources involved. The measurements done before for mocs_compilation.cpp and single source files builds though hint to overall build time reduction in real-world situations.

For some real-world numbers, a set of patches for a few KDE repos have been done (easy with the scripts available, see below). Then some scenario of someone doing a fresh build of such repo using the meta-build tool kdesrc-build was run a few times on an otherwise idle developer system (same i5-2520M CPU @ 2.5 GHz, with SSD), both for the current codebase and then with all possible moc includes added.

Using the make tool, configured to use 4 parallel jobs, with the build dir always completely removed before, and kdesrc-build invoked with the –build-only option, so skipping repo updates, the timing was measured using the time tool as before. Which reports by “real” the wall clock timing, while “user” reports the sum of times of all threads taken in non-kernel processor usage. The time spent by related kernel processing (“sys”) was ignored due to being very small in comparison.

The numbers taken in all cases showed that there clean builds got faster with moc includes, with build times partially reduced by more than 10 %:

Further, less controlled own time measurements for other codebases support this impression, as well as reports from others (“total build time dropped by around 10%.”, Qt Interest mailing list in 2019). With that for now it would be assumed that times needed for clean build are not a reason against moc includes, rather the opposite.

And there are more reasons, read on.

Reducing need for headers to include other headers

moc generated code needs to have the full declaration of types used as values in signals or slots method arguments. Same for types used as values or references for Q_PROPERTY class properties, in Qt6 also for types used with pointers:

class Bar; // forward declaration, not enough for moc generated code here

class Foo : public QObject {
    Q_OBJECT
    Q_PROPERTY(Bar* bar READ barPointer) // Qt6: full Bar declaration needed
    Q_PROPERTY(Bar& bar READ barRef)     // full Bar declaration needed
    Q_PROPERTY(Bar bar  READ barValue)   // full Bar declaration needed
Q_SIGNALS:
    void fooed(Bar bar); // full Bar declaration needed
public Q_SLOT:
    void foo(Bar bar);   // full Bar declaration needed
    // [...]
};

So if the moc file for class Foo is compiled separately and thus only sees the given declarations as above, if will fail to build.

This can be solved by replacing the forward declaration of class Bar with the full declaration, e.g. by including a header where Bar is declared, which itself again might need more declarations. But this is paid by everything else which needs the full class Foo declaration now also getting those other declarations, even if not useful.

Solving it instead by including the moc file in a source file with definitions of class Foo methods, with full class Bar declaration available there, as usually already needed for those methods, allows to keep the forward declaration:

#include "foo.h"
#include "bar.h" // needed for class Foo methods' definitions
// [definitions of class Foo methods]
#include "moc_foo.cpp" // moc generated code sourced

Which keeps both full and incremental project builds faster.

In KDE projects while making them Qt6-ready a set of commits with messages like “Use includes instead of forward decl where needed” were made, due to the new requirements by moc generated code with pointer types and properties. These would not have been needed with moc includes.

Enabling clang to warn about unused private fields

The clang compiler is capable to check and warn about unused private class members if it can see all class methods in the same compilation unit (GCC so far needs to catch up):

class Foo : public QObject {
    Q_OBJECT
    /* ... */
private:
    bool m_unusedFlag;
};

The above declaration will see a warning if the moc file is included with the source file having the definition of all (normal) non-inline methods:

/.../foo.h:17:10: warning: private field 'm_unusedFlag' is not used [-Wunused-private-field]
   bool m_unusedFlag;
        ^

But not if the moc file is compiled separately, as the compiler has to assume the other methods might use the member.

Better binary code, due to more in the compilation unit

A moc include into a source file provides the compiler with more material in the same compilation unit, which is said to be usable for some optimizations:

• “but also it produces slightly better code overall.” (Qt Interest mailing list, Dec. 2019)
• “Plus the benefit of more inlining, as there’s more the compiler can see.” (Qt Development mailing list, May 2020)

Indeed when building libraries in Release mode, so with some optimization flags enabled, it can be observed that size shrank by some thousandths for some. So at least size was optimized. For others though it grew a tiny bit, e.g. in the .text section with the code. It is assumed this is caused by the code duplications due to inlining. So there runtime is optimized at the cost of size, and one would have to trust the compiler for a sane trade-off, as done with all the other, normal code.

For another example, one of the commits to Qt’s own modules establishing moc includes for them reports in the commit message for the QtWidgets module:

A very simple way to save ~3KiB in .text[edit] size and 440b in data size on GCC 5.3 Linux AMD64 release builds.

So far it sounds like it is all advantages, so what about the disadvantages?

More manual code to maintain with explicit moc include statements

To have explicit include statements for each moc file covering a header (e.g. moc_foo.cpp for foo.h) means more code to manually maintain. Which is less comfortable.

Though the same is already the case for moc files covering source files (e.g. foo.moc for foo.cpp), those have to be included, given the class declarations they need are in that very source file. So doing the same also for the other type would feel not that strange.

The other manual effort needed is to ensure that any moc include is also done. At least with CMake’s automoc things will just silently work, any moc file not explicitly included is automatically included by the target’s mocs_compilation.cpp file. That one is currently always generated, built and linked to the target (TODO: file wish to CMake for a flag to have no mocs_compilation.cpp file).

One approach to enforce moc includes might be to add respective scripts as commit hooks, see. e.g. check-includemocs-hook.sh from KDAB’s KDToolBox.

No longer needed moc includes are also not critical with CMake’s automoc, an empty file will be generated and a warning added to the build log. So the developer can clean-up later when there is time.

So the cost is one include statement per moc-covered header and its occasional maintenance.

Automated moc file include statements addition, variant 6

There exist already a few scripts to scan sources and amend include statements for moc files where found missing, like:

• includemocs from the KDE Development Scripts repo (as old as 23 (sic!) years, see introducing commit)
• includemocs.pl & includemocs6.sh from Qt’s Qt Base repo util subdir
• Includemocs from KDAB’s KDToolBox repo
• fixMocInclude.sh from Remy van Elst’s blog post Add moc includes to speed up Qt compilation

Initially I was not aware of all. The ones tested (KDE’s, KDAB’s & Remy van Elst’s) missed to cover matching header files with the basename suffixed by “_p” (e.g. foo_p.h) to source files without that suffix (e.g. foo.cpp). So there is now a (working for what used for) draft of yet another script, addmocincludes. Oh dear

Suspicion substantiated: better use moc includes

As shown above, it looks that the use of explicit includes also for header moc files improves things for multiple stakeholders:

• developers: gain from faster full & incremental builds, more sanity check
• users: gain from runtime improvements
• CI: gains from faster full builds
• packagers: gain from faster full builds

All paid by the cost of one explicit include statement for each moc-covered header and its occasional maintenance. And in some cases a slightly bigger binary size.

Seems a good deal, no? So…

1. pick of one the scripts above and have it add more explicit moc includes
2. check for some now possible forward declarations
3. look out for any newly discovered unused private members
4. PROFIT!!! (enjoy the things gained long term by this one-time investment)

Update (Aug 14th):

To have the build system work along these ideas, two issues have now been filed with CMake’s issue tracker:

• Request: option to have automoc warn about non-explicit moc includes
• Request: option to have automoc not add a mocs_compilation.cpp file

This is the release schedule the release team agreed on

  https://community.kde.org/Schedules/KDE_Gear_23.08_Schedule

Dependency freeze is in less than 4 weeks (July 20) and feature freeze one
after that. Get your stuff ready!