Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce clang-format #249

Open
TheAssassin opened this issue Jun 18, 2022 · 4 comments · May be fixed by #250
Open

Introduce clang-format #249

TheAssassin opened this issue Jun 18, 2022 · 4 comments · May be fixed by #250

Comments

@TheAssassin
Copy link
Member

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
Copy link
Contributor

robalni commented Jun 18, 2022

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

@TheAssassin
Copy link
Member Author

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
Copy link
Contributor

robalni commented Jun 18, 2022

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
Copy link
Member

voidanix commented Jun 18, 2022

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:

  • Spaces and indentation for and around if statements
  • Spaces around operators
  • Indentation with long argument lists or repetitive statements
  • Character limit
  • Indentation with switch statements
  • Pointer declarations

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.

@voidanix voidanix linked a pull request Aug 10, 2022 that will close this issue
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 a pull request may close this issue.

3 participants