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

Update clang format rules #1215

Merged

Conversation

notlesh
Copy link
Contributor

@notlesh notlesh commented Apr 3, 2020

This updates clang-format rules. Unfortunately, since clang-format aggressively enforces its rules (it is never tolerant; that is, for any given rule, it always enforces some style), this change alone would require an enormous diff to the codebase (a "git blame wall", as Jeff called it).

So this also introduces git-clang-format (part of the typical clang-format distribution) and a git pre-commit hook script found in the following submodule:

external/clang-format-hooks -> https://github.com/barisione/clang-format-hooks/

This (currently) requires a one-time setup per clone:

# from repo root
# also, after e.g.
# git submodule update --init --recursive

./external/clang-format-hooks/git-pre-commit-format install

# this seems to use whatever `clang-format` it finds, so if `clang-format` resolves to
# the wrong version (e.g. probably version 9 on recent Ubuntu), adjust accordingly:
sudo update-alternatives --install /usr/bin/clang-format clang-format /usr/bin/clang-format-8 100

# or perhaps add a symlink in your $PATH to the desired version, or an alias...

# the environment variable $CLANG_FORMAT can also be set to the desired binary

Once this is done, any git commit will pause with an interactive question about applying the clang-format rules. Example:

git commit -va
--- daemon/main.cpp	(before formatting)
+++ daemon/main.cpp	(after formatting)
@@ -27,8 +27,10 @@
 handle_signal(int sig)
 {
   // omg no more explosion of template braces
-  std::set< std::string > > its2020afterAll;
-  its2020afterAll.insert("Some really, really long string but this time without the profanity. Keep your stick, though.");
+  std::set<std::string>> its2020afterAll;
+  its2020afterAll.insert(
+      "Some really, really long string but this time without the profanity. Keep your stick, "
+      "though.");

   if(ctx)
   {

The staged content is not formatted correctly.
The fix shown above can be applied automatically to the commit.

You can:
 [a]: Apply the fix
 [f]: Force and commit anyway (not recommended!)
 [c]: Cancel the commit
 [?]: Show help

What would you like to do? [a/f/c/?]

Remaining tasks:

  1. Avoid the one-off install mentioned above. Worst case: invoke from Makefile?
  2. Modify CI to use git-clang-format so that it only enforces relevant changes.

.clang-format Outdated

# when wrapping function calls/declarations, force each parameter to have its own line
BinPackParameters: 'true'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example:

void foo(int arg1,
         int arg2,
         int arg3);

Notice that this isn't allowed with this rule:

void foo(int arg1,
         int arg2, int arg3);

If such a declaration/function call wraps, this rule enforces that each param is on its own line. Feel free to object.

@notlesh notlesh force-pushed the update-clang-format-rules-2020-04-02 branch 2 times, most recently from 21f236b to 8ed4631 Compare April 3, 2020 20:51
@notlesh
Copy link
Contributor Author

notlesh commented Apr 3, 2020

This is a good resource for anyone who wants to play with style rules:

https://zed0.co.uk/clang-format-configurator/

@notlesh
Copy link
Contributor Author

notlesh commented Apr 3, 2020

Note: the pre-commit hook mentioned above will use whatever version clang-format happens to resolve to -- see the original message for workarounds (since lokinet uses version 8, which is superceded by version 9 and 11 now)

@notlesh
Copy link
Contributor Author

notlesh commented Apr 4, 2020

TODO:

clang-format-8 and clang-format-9 seem to disagree on some corner cases where a function declaration has both a template declaration and a return type. I haven't found the magic incantation to get the hook to use a specific version (it may involve both clang-format and clang-format-diff).

However, it seems that version 9 has correct behavior -- so perhaps we want to move to clang-format-9 anyway.

This submodule provides git hooks which invoke clang-format
in intelligent ways.
This commit reflects changes to clang-format rules. Unfortunately,
these rule changes create a massive change to the codebase, which
causes an apparent rewrite of git history.

Git blame's --ignore-rev flag can be used to ignore this commit when
attempting to `git blame` some code.
@notlesh notlesh force-pushed the update-clang-format-rules-2020-04-02 branch from 0b35491 to 2732709 Compare April 7, 2020 18:39
@notlesh notlesh requested a review from majestrate April 7, 2020 18:40
@majestrate majestrate merged commit 30d0eb5 into oxen-io:dev Apr 7, 2020
@notlesh notlesh mentioned this pull request Apr 7, 2020
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 this pull request may close these issues.

None yet

2 participants