blue-nebula / base

Main repository of Blue Nebula, a fast-paced shooter with a unique parkour system. It is a fork of Red Eclipse and is free software.
https://blue-nebula.org
15 stars 6 forks source link

Introduce clang-format #249

Open TheAssassin opened 2 years ago

TheAssassin commented 2 years ago

I think big PRs like #248 show that a code formatter and code format linter would greatly help improve the code quality within this repository. Since the initial fork over two years ago, I have worked with clang-format in a few projects, and I like how it works. It can auto-format only changed areas, sort headers, automatically lint changes (e.g., in PRs), easily be used in a pre-commit hook, etc.

In my opinion, we should codify the future code style for the project in a clang-format config included in this repository, and enforce its use during PRs using a separate GitHub action. This will save time and increase the readability quite a bit, thus saving developer time.

What do you think?

robalni commented 2 years ago

Do you want to change the style of the code or configure it to use the existing style as much as possible?

TheAssassin commented 2 years ago

I'd start by mimicking the current style, but fix some issues right away, e.g.,

# from
if(a == b) break;

# to
if (a == b)
{
    break;
}

We can and should talk about things like where to place the opening bracket (same line vs. separate line) or whether to have a space between if/for/... and the condition.

The idea is to apply these changes only to code modified within the scope of commits anyway, so even breaking code style changes (e.g., changing the spacing) will not bloat the amount of lines changed significantly. I do not intend to reformat the entire code base, since the amount of lines to be reviewed would be way too large.

robalni commented 2 years ago

I think most tools are fine to use as long as exceptions are allowed where a human thinks it's better to do it differently. Like if you have many small if-statements in a row I think it can be more readable to make them one line each, like:

if      (s == "a") do_a();
else if (s == "b") do_b();
else if (s == "c") do_c();
// ...
else print_error("Unknown action");

Another example; I usually put the opening brace on the same line in other projects, but if the head part gets so long that it's split into multiple lines, I think it's much more readable to put it on the next line:

for (int i = 0; i < 5; i++) {
    printf("Hello world\n");
}

// Looks bad
for (const struct ListNode *item = my_list.first;
     item != NULL;
     item = item->next) {
    printf("Hello world\n");
}

// Better
for (const struct ListNode *item = my_list.first;
     item != NULL;
     item = item->next)
{
    printf("Hello world\n");
}

Or if your style is to not put spaces after if, you might still want to do that sometimes to align code like in my first code box after the first if.

This all means that it's good to be careful with making automatic formatting too automatic or mandatory.

voidanix commented 2 years ago

clang-format would be cool indeed, but before jumping into actually writing rules for it, the current codestyle wiki entry must be updated for cases like the ones mentioned above.

For a tasklist:

FWIW, I actually prefer using Linux's or FreeBSD's codestyle because they are both very clearly defined and easy to use/understand, but ofc to each his own.