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

Agree on formatting style, and then create .clang-format config file to enforce it #1442

Open
seanm opened this issue Jan 22, 2024 · 17 comments
Labels
Misc Whitespace, typo, coding style, etc

Comments

@seanm
Copy link
Contributor

seanm commented Jan 22, 2024

We should agree on a formatting style.

Some files indent with tabs, some with spaces. Some put braces one way, some another.

Once we agree on a style, we can then autoformat using the clang-format tool. There are even tools that examine a codebase and generate a preliminary clang-format config file based on the predominant style.

@tormodvolden tormodvolden added the Misc Whitespace, typo, coding style, etc label Jan 22, 2024
@seanm
Copy link
Contributor Author

seanm commented Jan 23, 2024

I started some work here: #1443

First thing to decide on: tabs vs spaces :)

@Youw
Copy link
Member

Youw commented Jan 23, 2024

I think we will not be able to get a single .clang-format for the entire project. At least not at the beginning.
We need to figure out if it is possible to have several flavours of it at least per-platform.
So we can select "tabs vs spaces" at least per-plaform based on how it is mostly used right now.

@Youw
Copy link
Member

Youw commented Jan 23, 2024

Second thing: I think we should come up with a .clang-format, which, when applied - produces (reasonably) the smallest diff in the existing code.

@seanm
Copy link
Contributor Author

seanm commented Jan 23, 2024

We need to figure out if it is possible to have several flavours of it at least per-platform.

Why is that desirable? Such a small project could surely have a uniform style.

Second thing: I think we should come up with a .clang-format, which, when applied - produces (reasonably) the smallest diff in the existing code.

I did that already, using https://github.com/mikr/whatstyle. The result is the first commit here.

@Youw
Copy link
Member

Youw commented Jan 23, 2024

Why is that desirable?

To preserve git history - it matters:
image

If we apply .clang-format to all existing code - a lot of immediate blame/history will be hidden under something like "applying code style".
If we agree but don't apply format to all existing code - any change to places where something (e.g. tags vs spaces) is different would introduce lots of weird inconsistencies. I've seen that - looks terrible.

I did that already,

I meant per-backend, or even per-file.

@seanm
Copy link
Contributor Author

seanm commented Jan 23, 2024

If we apply .clang-format to all existing code - a lot of immediate blame/history will be hidden under something like "applying code style".

Which is why git blame has --ignore-rev and --ignore-revs-file. We can create a file listing all revisions that should be ignored in blame and even put that in the .gitconfig file.

@Youw
Copy link
Member

Youw commented Jan 23, 2024

Interesting, this option is only recently supported by Github itself.
That is actually a gamechanger (at least to me).

@tormodvolden
Copy link
Contributor

Second thing: I think we should come up with a .clang-format, which, when applied - produces (reasonably) the smallest diff in the existing code.

I did that already, using https://github.com/mikr/whatstyle. The result is the first commit here.

When doing this I think some oddballs in os/* should be taken out. Maybe that whole folder.

@tormodvolden
Copy link
Contributor

We need to figure out if it is possible to have several flavours of it at least per-platform.

Why is that desirable? Such a small project could surely have a uniform style.

There is code mainly worked on by people deeply rooted in their own platform style. I wouldn't mind having the Windows code Windows style and the openbsd code openbsd style. OTOH we should expect their contributors to be able to adapt to our common style also.

@seanm
Copy link
Contributor Author

seanm commented Feb 5, 2024

There is code mainly worked on by people deeply rooted in their own platform style. I wouldn't mind having the Windows code Windows style and the openbsd code openbsd style.

So let's see, that makes:

  • linux style
  • darwin style
  • OpenBSD style
  • Haiku style
  • android style
  • SunOS style
  • NetBSD style
  • core / crossplatform style

All that for a project with a mere 45 source files?

It is possible to have different .clang-format files per directory, but a lot of these variants are all together in the os folder.

It seems the choices are:

  1. forget about clang-format
  2. agree on a single libusb style
  3. reorganize files into different folders so they can have different styles

I vote for 2 personally. I don't care what the style is. Being able to auto-apply style makes contributing to the project easier, and it makes code reviews easier.

@Youw
Copy link
Member

Youw commented Feb 6, 2024

Optione 3 is something that hasn't been on the table and looks interesting.
Does git preserves a file history when it is moved to a different folder?

@seanm
Copy link
Contributor Author

seanm commented Feb 6, 2024

Optione 3 is something that hasn't been on the table and looks interesting.

Perhaps we should consider how many differences there actually are? If few, then moving files around may be more disruptive than (for example) just standardizing on tabs vs spaces.

Does git preserves a file history when it is moved to a different folder?

Subversion does, so surely git does too.

@Youw
Copy link
Member

Youw commented Feb 6, 2024

Subversion does, so surely git

I wouldn't be so sure - need to check.

@tormodvolden
Copy link
Contributor

I would be opposed to option 3, if clang-format cannot do better then I would rather put "// clang-format off" into the odd-ball files, if we want to keep them odd-ball.

produces (reasonably) the smallest diff in the existing code.

And again I think this exercise needs to be redone ignoring the current oddballs, or all os/* files. I don't think e.g. the haiku files should have a "vote" in our .clang-format specification.

@seanm
Copy link
Contributor Author

seanm commented Feb 13, 2024

And again I think this exercise needs to be redone ignoring the current oddballs, or all os/* files. I don't think e.g. the haiku files should have a "vote" in our .clang-format specification.

OK can do. What are the current oddballs? Other can haiku what shall I exclude?

@tormodvolden
Copy link
Contributor

To not hurt any feelings you could exclude all os/* :) But I think the events_*.c threads_*.c linux_*.c files are pretty consistent with our core files.

@seanm
Copy link
Contributor Author

seanm commented Feb 16, 2024

@tormodvolden ok see new commits, and commit messages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Misc Whitespace, typo, coding style, etc
Projects
None yet
Development

No branches or pull requests

3 participants