Skip to content

Add .clang-format file for consistent code formatting#1146

Open
deckstose wants to merge 2 commits intoaristocratos:mainfrom
deckstose:push-xuxxxzunnlnq
Open

Add .clang-format file for consistent code formatting#1146
deckstose wants to merge 2 commits intoaristocratos:mainfrom
deckstose:push-xuxxxzunnlnq

Conversation

@deckstose
Copy link
Copy Markdown
Collaborator

@deckstose deckstose commented May 6, 2025

This does not aim to be a config to do the least formatting compared to the current code base, it tries to be readable wherever possible.

This required some manual reordering of headers since preprocessor statements in between header declaration don't play nice with header sorting.

Closes: #573

@deckstose deckstose requested a review from aristocratos May 6, 2025 20:32
@deckstose deckstose force-pushed the push-xuxxxzunnlnq branch 7 times, most recently from cbd935c to d21f0c1 Compare May 6, 2025 22:26
@deckstose deckstose marked this pull request as draft May 6, 2025 22:43
@deckstose deckstose force-pushed the push-xuxxxzunnlnq branch from d21f0c1 to f192c25 Compare May 6, 2025 22:59
@deckstose deckstose marked this pull request as ready for review May 6, 2025 23:01
@aristocratos
Copy link
Copy Markdown
Owner

I don't see a good reason to enforce formatting.

Formatting aren't usually what makes code hard to read, more so coding style and weird naming conventions.

For example the suggested column limit of 120 characters are broken in most if not all files in this repository (which is fine in my opinion, the vast majority of people are on 1080p an up).

@deckstose
Copy link
Copy Markdown
Collaborator Author

For example the suggested column limit of 120 characters are broken in most if not all files in this repository (which is fine in my opinion, the vast majority of people are on 1080p an up).

This can be increased. I don't care about the specific style, just that it is consistent. Having some parameters align on the next line of a function with local variables makes by brain go 💥

btop/src/btop_shared.cpp

Lines 181 to 186 in 9bd618f

void _tree_gen(proc_info& cur_proc, vector<proc_info>& in_procs, vector<tree_proc>& out_procs,
int cur_depth, bool collapsed, const string& filter, bool found, bool no_update, bool should_filter) {
auto cur_pos = out_procs.size();
bool filtering = false;

Take this for example, where does the parameter list end and where do the function local variables begin? It is harder to read.

Naming conventions could be enforced as well (like CamelCase struct names and lower_case variable/function names).

I don't see a good reason to enforce formatting.

I know we had this discussion before and you said you don't have strong feelings about it. What would be the harm to try this out? And what would be a good reason to not use the same formatted code for new contributions?

@aristocratos
Copy link
Copy Markdown
Owner

If you follow the git blame back for _tree_gen to the original you'll see that it was one line originally that later was changed, likely because of the committers own clang-format.

What would be the harm to try this out?

Well, I guess waste of energy when all the github actions has to rerun every time someone has to correct their formatting to pass our .clang-format.

What I mean to say is that formatting aren't really that important.
And in the few cases where they are, it's not that hard to just point it out to the PR author.

@deckstose
Copy link
Copy Markdown
Collaborator Author

Well, I guess waste of energy when all the github actions has to rerun every time someone has to correct their formatting to pass our .clang-format.

If we check in the .clang-format file this will hint to most contributors that there is some style guide (we can also add a notice) to follow and will make it easier on the developer side to already format the file before contributing back. If we trust the stack overflow survey then most people will be using VSCode which has clang-format integration in it's Cpp plugin. It will integrate well within the workflow of most people.

What I mean to say is that formatting aren't really that important.

It is. IMO even on more then variable naming. I don't know your code as well as you do, I (voluntarily 😃) need to look at it and the newly contributed code and understand what is happening. Any deviation of where I expect a certain symbol to be makes it harder. This will also make spotting mistakes easier, since the code format is not something to worry about (it's the same, and predictable, everywhere).

And in the few cases where they are, it's not that hard to just point it out to the PR author.

This is tedious. Why not let the machine handle it? That's what they're good at.

@aristocratos

This comment was marked as resolved.

@deckstose
Copy link
Copy Markdown
Collaborator Author

Sure, if you can make sure this doesn't happen:

Will work on this 🕵🏽

In essence someone making a single change in this file and then running format document would touch about half the lines in the file. Making git blame essentially useless.

That's not right. There is git-clang-format, which is used in the workflow as well. There is also .git-blame-ignore-revs which is supported by Github's UI to ignore commits of mass re-formatting.

I would say the output is far from an improvement, some examples:

It's not useful in all cases. These block can be ignored with // clang-format on/off.

@deckstose
Copy link
Copy Markdown
Collaborator Author

Sure, if you can make sure this doesn't happen:

I guess this is unavoidable. It just shows that there is a lot of inconsistency at the moment.

@deckstose deckstose marked this pull request as draft November 8, 2025 11:26
@deckstose deckstose force-pushed the push-xuxxxzunnlnq branch 3 times, most recently from 39bd2fd to 965fa8c Compare December 23, 2025 18:18
@deckstose deckstose marked this pull request as ready for review January 20, 2026 10:11
@deckstose
Copy link
Copy Markdown
Collaborator Author

I've given this another shot. I've found the toggle for braced initializer lists so that config and menu mappings don't look completely f-ed up. Formatting is not applied right now, but when we do, we should add a file afterwards to tell git blame to ignore the commit.

@vandabbin
Copy link
Copy Markdown
Contributor

I've given this another shot. I've found the toggle for braced initializer lists so that config and menu mappings don't look completely f-ed up. Formatting is not applied right now, but when we do, we should add a file afterwards to tell git blame to ignore the commit.

Personally I'm all for this. My editor (neovim) really really doesn't like the indentation of the namespaces and it is a constant annoyance. For some reason even though I have the config setting in neovim sets that it should be allowing this indentation. It just ignores it. Making me think a plugin is screwing with it. I bet a good .clang_format file would solve it though.

@deckstose
Copy link
Copy Markdown
Collaborator Author

It always indents to the start of the line even if it should indented right? 😆

@vandabbin
Copy link
Copy Markdown
Contributor

Yup exactly! 😆

@deckstose
Copy link
Copy Markdown
Collaborator Author

deckstose commented Jan 21, 2026

I hope I addressed all the issues raised in #1146 (comment)

If someone else wants to take a look at the formatted files and see if there is anything obvious that's totally out of line, I'm glad to fix it

@aristocratos
Copy link
Copy Markdown
Owner

Get this error when testing this with clang-format 18.1.8 (latest available in the regular repositories in Ubuntu 24.04)

.clang-format:19:20: error: invalid boolean
BinPackParameters: OnePerLine
                   ^~~~~~~~~~

Also, why is the PR removing a bunch of comments?

@deckstose
Copy link
Copy Markdown
Collaborator Author

deckstose commented Jan 21, 2026

Also, why is the PR removing a bunch of comments?

I have done the edits in work tree with a bunch of other staging edits, maybe they slipped in when I split them up. However: I didn't see their usefulness, as none of the other provided headers has any explanation why they where added (and IDE tools can provide you that information anyway). As it's unrelated, I will remove them 🐦

And the order of <net/if.h> and <ifaddrs.h> will be managed by clang-format`. Makes me wonder if we should add the formatting to this PR (or right after). Not sure.

Get this error when testing this with clang-format 18.1.8 (latest available in the regular repositories in Ubuntu 24.04)

We don't support LLVM 18 anymore. I can recommend the https://apt.llvm.org/ site, this is how we get the LLVM releases in CI as well. Otherwise a container will work or I can push a branch with the formatted code, if you want me to.

@deckstose
Copy link
Copy Markdown
Collaborator Author

I've added a commit with the formatted changes.

@deckstose deckstose force-pushed the push-xuxxxzunnlnq branch 2 times, most recently from 005baea to 4cb2560 Compare February 2, 2026 10:42
@aristocratos

This comment was marked as resolved.

@deckstose

This comment was marked as resolved.

@aristocratos

This comment was marked as resolved.

@deckstose

This comment was marked as resolved.

@aristocratos
Copy link
Copy Markdown
Owner

I still don't think this is an improvement.
It also weirdly seems inconsistent with what is split up and what is compacted.

Function calls are split over multiple rows even if there's just 2-3 arguments.
Meanwhile some structs are compacted to be one-liners even if there is like 6-7 variables declared.

Any manually created structure in initializer-lists are completely destroyed and ends up worse.

And do really an if statement like for example if (bool) var = 5 really need to split up in to two rows? That's just a waste of space (or newline in this particular case...).

The only actual fixes I see is the small changes like correcting indent characters and indentation, moving starting braces and moving the reference/pointer symbols.

@deckstose
Copy link
Copy Markdown
Collaborator Author

Function calls are split over multiple rows even if there's just 2-3 arguments.
Meanwhile some structs are compacted to be one-liners even if there is like 6-7 variables declared.

Can you point me to a specific part so I can take a look. This is configurable.

At the moment the tool tries to fit arguments on a single line given the max line length, and if it doesn't fit it will split it up into one arg per line.

@deckstose deckstose force-pushed the push-xuxxxzunnlnq branch 3 times, most recently from 1a23585 to 4754137 Compare February 7, 2026 14:07
@deckstose
Copy link
Copy Markdown
Collaborator Author

And do really an if statement like for example if (bool) var = 5 really need to split up in to two rows? That's just a waste of space (or newline in this particular case...).

"Fixed"

Comment thread src/btop_shared.hpp
@deckstose deckstose force-pushed the push-xuxxxzunnlnq branch 2 times, most recently from 27f255f to 6a533c5 Compare February 11, 2026 20:46
@deckstose deckstose force-pushed the push-xuxxxzunnlnq branch from 6a533c5 to e4b4cb7 Compare March 4, 2026 10:44
@deckstose deckstose force-pushed the push-xuxxxzunnlnq branch from e4b4cb7 to 86e06e4 Compare March 4, 2026 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Development] Add .clang-format file

3 participants