search

hse-project / hse

HSE: Heterogeneous-memory storage engine
https://hse-project.github.io
673 stars 65 forks source link

alexttx/clang format #581

Closed alexttx closed 1 year ago

alexttx commented 1 year ago

Round 4:

Round 3 updates

Round 2 updates

Round 1: Initial stab w/ clang-format v13

Here's how you would run clang-format on the entire source tree:

$ git ls-files | grep '\.[ch]$' | xargs clang-format -i --style=file --fallback-style=none

Clang-format documentation: https://clang.llvm.org/docs/ClangFormatStyleOptions.html

tristan957 commented 1 year ago

For anyone else struggling to review this on GitHub, I am using VSCode to make comments.

tristan957 commented 1 year ago

Alex, let's make sure to not format the code in the built-in subprojects like hyperloglog, crc32c, rbtree, and xoroshiro.

alexttx commented 1 year ago

Alex, let's make sure to not format the code in the built-in subprojects like hyperloglog, crc32c, rbtree, and xoroshiro.

I used git ls-files which only lists files git knows about. I'm not yet sure how this integrates with development and CI.

tristan957 commented 1 year ago

Those built-in subprojects are also tracked in Git, so you need to exclude them.

tristan957 commented 1 year ago

I changed the doc links in the PR description to point the canonical source that has version indicators of when options were introduced.

tristan957 commented 1 year ago

For others. I found this site which seems pretty cool: https://clang-format-configurator.site/

beckerg commented 1 year ago

Ready for next iteration?

alexttx commented 1 year ago

Ready for next iteration?

I don't see any suggested changes. So this is it?

Or is next round to format the entire code base?

tristan957 commented 1 year ago

Seems like you left a few todos in the clang-format file. Might as well just run it over the whole code base next.

tristan957 commented 1 year ago

Alternatively change the input file to something more substantial like what https://clang-format-configurator.site/ has. Granted it is C++, but still

beckerg commented 1 year ago

I need to review all asm it touches to ensure it doesn't eff it up. But then I don't think we have much asm anymore...

alexttx commented 1 year ago

Seems like you left a few todos in the clang-format file. Might as well just run it over the whole code base next.

Right. I was dragging my feet on the include file sorting.

The HSE_LIKELY macro is fine. It is formatted like a function call which is appropriate given its use.

You and others are welcome to push an update. Using the same example as https://clang-format-configurator.site/ is a great idea. Go for it!

beckerg commented 1 year ago

What's the default for switch case label indention? Should be off..

IndentCaseLabels: false

alexttx commented 1 year ago

What's the default for switch case label indention? Should be off..

Neither setting gets what you want.

https://clang.llvm.org/docs/ClangFormatStyleOptions.html#indentcaselabels

alexttx commented 1 year ago

@beckerg , see https://github.com/hse-project/hse/pull/581/commits/c76cb733a9584a68b1329c3f1f44e2e15c78273e

tristan957 commented 1 year ago

I guess we seem pretty ready to just run format over the whole codebase and check it out?

alexttx commented 1 year ago

I'm tearing down this PR. Will replace with two PRs. One that has prep work to add escapes where needed and where desired, another to to the reformat.