Add .clang-format #1866
Add .clang-format #1866
Conversation
|
While we have this .clang-format file, it might be worth discussing what to do with it going forward. We should consider checking if the PRs are properly formatted. We could also consider enforcing this as a procedure when creating PRs. I don't want people to "fight the formatter" or spend too much time making the code format well with clang-tidy. There's no hard requirement to never disable clang-format, as long as they're reasonable. |
There was a problem hiding this comment.
Main things I don't really like:
- It tries to align stuff with tons of spaces. I could understand a few spaces, but in some places it's more spaces than actual code...
- It merges small if blocks into a single line
- It merges small functions into a single line
I think we should avoid doing a big commit with all these changes, and just add them when we touch the respective code.
I understand that it might be a bit of a chore, but I think a big commit like this makes git blame more annoying to work with.
Still, having a .clang-format that has a final say on what the formatting should be is a good thing.
src/api/dirmonitor/fsevents.c
Outdated
| static void stream_callback(ConstFSEventStreamRef streamRef, | ||
| void *monitor_ptr, | ||
| size_t numEvents, | ||
| void *eventPaths, | ||
| const FSEventStreamEventFlags eventFlags[], | ||
| const FSEventStreamEventId eventIds[]) { |
There was a problem hiding this comment.
I'm not sure I like this. What other formatting alternatives are there?
There was a problem hiding this comment.
It's pretty this or align the parameters on a newline.
takase1121
force-pushed
the
chore/clang-format
branch
2 times, most recently
from
b179e9b
to
ecc83e1
Compare
|
This formatting sucks overall and is super (badly) opinionated. There's no problems with readability in the current codebase so this commit is completely unnecessary. The inconsistencies in the codebase can be fixed manually and instead of forcing formatting, put the coding style in the contributing doc. |
I would greatly appreciate constructive feedback on the formatting. I believe formatting code is indeed an opinionated (and mostly opinion-based) business. There should be minimal compromises (sticking to a known code style) or no compromises (like Black, which enforces a single style) for how code should look. Ultimately I am biased towards no-compromises formatting like Black and The intention of adding .clang-format is to fix consistencies in the codebase. Fixing the inconsistencies manually is possible but takes time, which could be better utilized to do something else. The whole point of having .clang-format is to remove the need to format code and make mistakes when doing so manually. I suggest you check out the PR in an editor and read it instead of using GitHub's diff view. That way, you would get complete context of the changed lines (by clang-format) and understand that the code really hasn't changed that much. There are some rough edges, but in my opinion, this is decently familiar with our current codebase. As @Guldoman suggested, the real issue here is how to deploy a code style, whether in the form of clang-format or documentation. If we merge this commit as-is (which I've warned not to do) it will cause issues with Git blame, so we may need to explore that. |
…ting This will certainly be a polarizing, if not controversial PR. I scanned the project with Clang Format Editor and edited some of the values to match the current project as close as possible. It will not be an exact match, but I think it is good enough.
takase1121
force-pushed
the
chore/clang-format
branch
from
ecc83e1
to
264671a
Compare
|
I experimented with aligning the pointer to the left instead of the right; it appears that we have about the same amount of code that aligns in both directions. |
This would probably be a polarizing, if not controversial, PR.
This PR adds .clang-format based on the current code style, which would format most source files except the UTF8 library since that library doesn't seem to have been created by us.
Please refer to the changelog in the individual commit to see what is actually being done.