[Maintainability] clang-format is making code less readable

[JimB123]

I'm a big fan of having some standard guidelines around code formatting. I'm also a fan of having automation check that these guidelines are being followed. But that's not what clang-format does.

clang-format reformats code based on a defined pattern. It's not just checking that a certain guideline is being followed. It decides exactly how the code should be formatted, and enforces that. In many cases, this is making the code less readable/maintainable.

From a quick search, it seems that most big open-source projects are using some type of lint tool to detect violations of guidelines. But most don't seem to use a formatter - and instead rely on code inspection for the final decision.

I was able to find an alternate tool AStyle which seems to focus on identifying poor patterns or styles deemed not acceptable rather than formatting into a specific fixed format. clang-format gives a choice: do it THIS way or do it THAT way, AStyle says this pattern is not allowed - but I won't mess with your other formatting.

Here are some examples of where clang-format is impacting readability: (Scroll right to see the corrections)

Original Code	Formatted Code
-typedef void (*iteratorReleaseFunc) (genericIterator *genIt); -typedef fifo * (*iteratorGetEntriesFunc) (genericIterator *genIt, int *orig_dbid, int *cur_dbid); -typedef void (*iteratorSwapDbFunc) (genericIterator *genIt, int db1, int db2); -typedef void (*iteratorFlushDbFunc) (genericIterator *genIt, int cur_dbid); -typedef bool (*iteratorHasPassedItemFunc) (genericIterator *genIt, const_sds key, int cur_dbid); -typedef int (*iteratorOriginalDbFunc) (genericIterator *genIt, int cur_dbid); -typedef bool (*iteratorIsKeyInScopeFunc) (genericIterator *genIt, const_sds key);	+typedef void (*iteratorReleaseFunc)(genericIterator *genIt); +typedef fifo *(*iteratorGetEntriesFunc)(genericIterator *genIt, int *orig_dbid, int *cur_dbid); +typedef void (*iteratorSwapDbFunc)(genericIterator *genIt, int db1, int db2); +typedef void (*iteratorFlushDbFunc)(genericIterator *genIt, int cur_dbid); +typedef bool (*iteratorHasPassedItemFunc)(genericIterator *genIt, const_sds key, int cur_dbid); +typedef int (*iteratorOriginalDbFunc)(genericIterator *genIt, int cur_dbid); +typedef bool (*iteratorIsKeyInScopeFunc)(genericIterator *genIt, const_sds key);
struct genericIterator { - iteratorReleaseFunc release; - iteratorGetEntriesFunc getEntries; - iteratorSwapDbFunc swapDb; - iteratorFlushDbFunc flushDb; - iteratorHasPassedItemFunc hasPassedItem; - iteratorOriginalDbFunc originalDb; - iteratorIsKeyInScopeFunc isKeyInScope; };	struct genericIterator { + iteratorReleaseFunc release; + iteratorGetEntriesFunc getEntries; + iteratorSwapDbFunc swapDb; + iteratorFlushDbFunc flushDb; + iteratorHasPassedItemFunc hasPassedItem; + iteratorOriginalDbFunc originalDb; + iteratorIsKeyInScopeFunc isKeyInScope; };
- return (!it->completed - && !it->terminated - && mutexQueueLength(it->items_for_iterator) < it->item_count_target);	+ return (!it->completed && !it->terminated && mutexQueueLength(it->items_for_iterator) < it->item_count_target);
- serverAssert(it->dbentries_queued >= it->dbentries_processed); - serverAssert(it->replication_queued >= it->replication_processed); - serverAssert(it->swapdb_queued >= it->swapdb_processed); - serverAssert(it->flushdb_queued >= it->flushdb_processed);	+ serverAssert(it->dbentries_queued >= it->dbentries_processed); + serverAssert(it->replication_queued >= it->replication_processed); + serverAssert(it->swapdb_queued >= it->swapdb_processed); + serverAssert(it->flushdb_queued >= it->flushdb_processed);
static bool expediteSingleKeyWithoutOptimization( - bgIterator *it, - int dbid, - robj *oKey, - dict *waitingOnKeys) { - bool mustBlock = false;	static bool expediteSingleKeyWithoutOptimization( + bgIterator *it, + int dbid, + robj *oKey, + dict *waitingOnKeys) { bool mustBlock = false;
- if (!(iterComplete || it->keyset_iter->hasPassedItem(it->keyset_iter, key, dbid)) - && (dictFind(it->early_iterate_entries, de) == NULL)) {	+ if (!(iterComplete || it->keyset_iter->hasPassedItem(it->keyset_iter, key, dbid)) && (dictFind(it->early_iterate_entries, de) == NULL)) {
- if (!(iterComplete || it->keyset_iter->hasPassedItem(it->keyset_iter, key, dbid)) - && dictFind(it->early_iterate_entries, de) == NULL - && ((bgIterationEntryMetadata *)objectGetMetadata(de))->iterator_epoch <= it->consistent_modification_id) {	+ if (!(iterComplete || it->keyset_iter->hasPassedItem(it->keyset_iter, key, dbid)) && dictFind(it->early_iterate_entries, de) == NULL && ((bgIterationEntryMetadata *)objectGetMetadata(de))->iterator_epoch <= it->consistent_modification_id) {
- status->dbentries_queued = it->dbentries_queued; - status->dbentries_processed = it->dbentries_processed; - status->replication_queued = it->replication_queued; status->replication_processed = it->replication_processed; - status->swapdb_queued = it->swapdb_queued; - status->swapdb_processed = it->swapdb_processed; - status->flushdb_queued = it->flushdb_queued; - status->flushdb_processed = it->flushdb_processed;	+ status->dbentries_queued = it->dbentries_queued; + status->dbentries_processed = it->dbentries_processed; + status->replication_queued = it->replication_queued; status->replication_processed = it->replication_processed; + status->swapdb_queued = it->swapdb_queued; + status->swapdb_processed = it->swapdb_processed; + status->flushdb_queued = it->flushdb_queued; + status->flushdb_processed = it->flushdb_processed;

[madolson]

I generally like the transition to clang-format. It forces a stricter style, but I don't think that is a bad thing overall.

Of the list here, the only one I strongly agree with (most I'm neutral or disagree with), is the extremely long line lengths. In your specific case:

-            if (!(iterComplete || it->keyset_iter->hasPassedItem(it->keyset_iter, key, dbid))
-                    && dictFind(it->early_iterate_entries, de) == NULL
-                    && ((bgIterationEntryMetadata *)objectGetMetadata(de))->iterator_epoch <= it->consistent_modification_id) {

I struggle to read this anyways. I would rather have intermediary booleans than this.

[madolson]

@valkey-io/core-team Anyone else have thoughts here?

[lucasyonge]

I think it is a little bit strange for the if condition statement: from the above example, the long if statement happen twice, but for the beginning of the function, the format is good,

static bool expediteSingleKeyWithoutOptimization(

• bgIterator *it,
• int dbid,
• robj *oKey,
• dict *waitingOnKeys) {
  bool mustBlock = false;

I check with chatgpt, it answers me as below:

clang-format making if statements very long after formatting is usually not a bug. It’s following its formatting rules, especially those that try to minimize line breaks and keep code compact. Common causes include:

1. AllowShortIfStatementsOnASingleLine

This is the most common reason
If enabled: AllowShortIfStatementsOnASingleLine: true
or AllowShortIfStatementsOnASingleLine: AllIfsAndElse

2. ColumnLimit is too large (or set to 0)
3. BinPackConditions This packs multiple conditions into one line:
4. BreakBeforeBinaryOperators

A commonly recommended configuration:

AllowShortIfStatementsOnASingleLine: false
ColumnLimit: 100
BinPackConditions: false
BreakBeforeBinaryOperators: All

This produces:

if (a &&
b &&
c)
{
doSomething();
}

I am not sure if it helps

[JimB123]

The issue is that clang-format decides on one, and only one, correct format for a statement.

Yes, you can change config to affect the way that clang decides on the one specific format. But in the end, there is zero option for the developer to change even a single white-space. There is no option for a human to decide where the line splits make things more readable - or where additional spacing may make things more readable. (Unless you litter the code with statements to disable clang.)

This is why most large open source projects don't use a formatter, opting instead for a linter mechanism to detect anti-patterns rather than enforcing an absolute/specific pattern.

[zuiderkwast]

But in the end, there is zero option for the developer to change even a single white-space. There is no option for a human to decide where the line splits make things more readable - or where additional spacing may make things more readable.

I agree that manually formatted code is better and is more readable, especially when I write the code myself. I often think clang-format is making my code uglier. But I think that is subjective.

For example, I don't like your examples with lots of vertical alignment. It isn't easier for me to read code that looks like a table. It does however have the downside that it requires the whole section to be reformatted when a line is modified in the middle if some column gets wider or narrower.

When reviewing other people's pull requests, I find that it saves much time and effort that formatting is no longer a topic. The reviewer can focus on the content. The formatting was a distraction that is now gone.

In the same way, I think this whole discussion is a distraction. Style changes back and forth doesn't move the project forward, only sideways (including changes like #3540). I would like us to move forward instead, and faster.

[harrylin98]

The tradeoff is essentially:

• clang-format: catches everything, but is too aggressive — kills intentional formatting along with the bad.
• AStyle: preserves developer intent, but also preserves developer mistakes. For example, int a = 10; would pass AStyle in all cases.

[JimB123]

When reviewing other people's pull requests, I find that it saves much time and effort that formatting is no longer a topic. The reviewer can focus on the content. The formatting was a distraction that is now gone.

In the same way, I think this whole discussion is a distraction. Style changes back and forth doesn't move the project forward, only sideways (including changes like #3540). I would like us to move forward instead, and faster.

I totally agree that nitpicking format in a code review is a waste of time.

One example that drives me nuts is that @madolson hates camelCaseVariables and wants everything to be snake_case. I tend to prefer the opposite, and naturally write code that way. She shouldn't have to pick on my variables in every code review. We should either decide that we don't care (and then she shouldn't comment) or we should decide that, as a project, we do care, and use a tool (not a document) to enforce it. I'm personally ok with either.

At the end of the day, I'm more in favor of a less prescriptive approach, and allowing the developer some choice when we don't really care. Use a tool to enforce the important stuff, but be less prescriptive about everything else.

... or maybe I just need to accept that an automated tool is going to make my code uglier, and learn not to care.