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

Add clang-format #639

Merged
merged 3 commits into from
Dec 24, 2019
Merged

Add clang-format #639

merged 3 commits into from
Dec 24, 2019

Conversation

danobi
Copy link
Member

@danobi danobi commented May 15, 2019

This patch adds clang-format to the code base.

This is a pretty big patch. I played around with the settings for a while and this seemed like a somewhat sane configuration. That being said, I don't really care what the settings are. I'd just like the codebase to be consistent.

If anyone has any opinions, now would be the time to bikeshed.

This closes #182 .

.travis.yml Outdated Show resolved Hide resolved
Copy link
Member

@ajor ajor left a comment

Choose a reason for hiding this comment

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

Thanks, it'd be good to have a consistant style - here comes the bikeshedding as requested!

src/arch/aarch64.cpp Outdated Show resolved Hide resolved
src/arch/aarch64.cpp Outdated Show resolved Hide resolved
src/arch/x86_64.cpp Outdated Show resolved Hide resolved
.clang-format Outdated Show resolved Hide resolved
.clang-format Outdated Show resolved Hide resolved
src/attached_probe.cpp Outdated Show resolved Hide resolved
tests/tracepoint_format_parser.cpp Outdated Show resolved Hide resolved
@mmarchini
Copy link
Contributor

I like consistent style as well. A few comments (haven't look at the results yet):

  1. On other projects where clang-format was enforced, I noticed that different versions of clang-format could result in different formatting. Is there a way to avoid that issue?
  2. Instead of applying clang-format to the entire code base (which will affect git history as well as cause a ton of conflicts with existing pull requests), what about we apply it gradually using something like https://github.com/llvm-mirror/clang/blob/master/tools/clang-format/git-clang-format to format only changes on the code touched by a commit? We can make Travis check the changes (and only the changes) are following our clang-format settings.

@danobi
Copy link
Member Author

danobi commented Jun 3, 2019

On other projects where clang-format was enforced, I noticed that different versions of clang-format could result in different formatting. Is there a way to avoid that issue?

On one project I worked on, there was a make clang-format rule that was configured to (1) download a specific clang-format release if not already downloaded and (2) run clang-format using that specific binary. I suppose we could have something like that.

what about we apply it gradually

That sounds very reasonable. My only concern is that not everything will be touched over time. But I suppose in that case we could start manually applying clang-format.

I'll carve out some time to work on this. Turns out tuning clang-format rules is not a trivial task :)

@fbs fbs mentioned this pull request Nov 24, 2019
Otherwise clang-format will start bin packing these parameters.
@danobi
Copy link
Member Author

danobi commented Dec 22, 2019

Just force pushed. I believe I've addressed all the outstanding comments. Now this PR will gradually guide PRs to the final format.

I did a full format of semantic_analyser.{h,cpp}: http://ix.io/255p . That file should be representative of the clang-format configuration.

@danobi danobi force-pushed the clang_format branch 5 times, most recently from 1eb8884 to 4218be1 Compare December 22, 2019 01:10
Any clang-format errors will appear as a separate build job. The
clang-format build job will also have a handy diff at the bottom.
@danobi
Copy link
Member Author

danobi commented Dec 22, 2019

Screenshot from 2019-12-21 17-23-57

This is what a format error looks like for the following change:

commit 3518b230dc1393fc5df00987d7fa9c11ac699f8b (HEAD -> clang_format, origin/clang_format)
Author: Daniel Xu <[email protected]>
Date:   Sat Dec 21 16:32:04 2019 -0800

    test change

diff --git a/src/resolve_cgroupid.cpp b/src/resolve_cgroupid.cpp
index 5bcc675..ce7ca12 100644
--- a/src/resolve_cgroupid.cpp
+++ b/src/resolve_cgroupid.cpp
@@ -1,3 +1,4 @@
+#include <iostream>
 #ifndef _GNU_SOURCE
 # define _GNU_SOURCE
 #endif
@@ -39,7 +40,7 @@ int name_to_handle_at(int dirfd, const char *pathname,
                       struct file_handle *handle,
                       int *mount_id, int flags)
 {
-  return (int)syscall(SYS_name_to_handle_at, dirfd, pathname, handle, mount_id, flags);
+  return (int)syscall(SYS_name_to_handle_at, dirfd, pathname, handle, mount_id, flags); std::cout << "help me 2!";
 }

 #endif

Copy link
Member

@ajor ajor left a comment

Choose a reason for hiding this comment

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

Nice thank you! All the settings look good but if we see it trying to make strange changes we can always just ignore the clang-format CI job, so I'll merge it now and see how it goes!

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.

Create a clang-format config
4 participants