Add clang-format file #5092

roligugus · 2020-06-23T00:26:49Z

clang-format allows to auto-format C code. The settings here are set up to follow the code style, see https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Coding_Style.

Some bike shedding should still happen. Includes sample formatted code.

DO NOT MERGE AS IS.
This is not about reformatting all source code. That would be another discussion.

Make sure these boxes are signed before submitting your Pull Request -- thank you.

I have read the contributing guide lines at https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Contributing
I have signed the Open Information Security Foundation contribution agreement at https://suricata-ids.org/about/contribution-agreement/
I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable)

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/3736

Describe changes

FOOD FOR THOUGHT, DO NOT MERGE AS IS
clang-format is the standard way of formatting C style code
This pull request add a .clang-format configuration file so that clang-format can be used by people that do not want to manually format code or set up their editors. See below for sample usage.
- Adding (minimal) .clang-format configuration implementing the Suricata code style. Best to discuss format through comments on the sample_formatted_code.do_not_merge.c file.
- Included also alternative "full" config not based on other project (llvm in our case), see clang-format.full.do_not_merge
Included sample C code file showing the formatting style as applied, see file sample_formatted_code.do_not_merge.c.
- Comments refer to the corresponding Suricata code style section where applicable. Other settings are interpretations of existing code, but I might have picked the wrong format out of multiple.
- Initial version was created using https://github.com/mikr/whatstyle but lots of manual configurations afterwards resulted in essentially a different config.

Discussion and decisions

Is this useful? I personally find it very useful as it helps avoiding formatting discussions during code review and it allows to enforce consistency. However, you guys might think differently.
One can set up a minimal .clang-format "based on an existing project's style" or specify all settings. Includes both for people to compare the approaches. I'd recommend to pick the diff-based one, but the all settings approach can also be useful.
Would it be useful to include the "sample formatted source code" somewhere? Parts of that should eventually make it into the "Code Style" wiki page.
Bikeshedding some of the settings? I ran it through a few files to see how much it changes. I could have missed some common code style.
If format is agreed upon, the Coding_Style wiki page could be updated.
- Doh, I just realized that the code style is also available at https://github.com/OISF/suricata/blob/master/doc/devguide/codebase/code-style.rst
- Should update that one and the wiki?

Requires

Requires clang, e.g. on fedora: sudo dnf install clang
Tested with clang 9: clang-format version 9.0.1 (Fedora 9.0.1-2.fc31) and latest trunk (clang 11)
Have not tested with older versions

How to use it?

You can limit the formatting to your changes only by using "git format-patch". Requires a separate package, e.g. on fedora: sudo dnf install git-clang-format. This works surprisingly well, but it might not always work as well as the context might be missing.
You can format the whole file if you want to go nuts

# git clang-format can work off commits, i.e. you commit all your changes before using it, format code, and then commit the formatting changes separately.
# 
# To format changes in all commits in your branch, e.g. if branched off master:
$ git clang-format master

# To format changes in last commit only
$ git clang-format HEAD^

# git clang-format can also work off staged changes, i.e. you need to git add your changed files as a minimum
# To format current changes:
$ git clang-format

# clang-format proper works off the file, not just your changes. It has more context.
#
# To format the whole file
$ clang-format -i {file}

# e.g. to format all source files. Don't do this. Lots of changes.
$ clang-format -i src/*.h src/*.c

Other Notes

Trailing empty lines (at end of file) are removed
One can always disable clang-format for sections where the formatting might not be to the liking.
- This probably applies mostly to macros, arrays (single or multi-dimensional ones), struct initialization, or where one manually formatted code as that might be quite different.
Rewriting of comments to fit to 80 chars length is disabled.
See notes in sample formatted file
clang-format settings documentation: https://clang.llvm.org/docs/ClangFormatStyleOptions.html

PRScript output (if applicable):
n/a

clang-format allows to auto-format C code. The settings here are set up to follow the code style, see https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Coding_Style. Some bike shedding should still happen. Includes sample formatted code. One can set up .clang-format as "differential setting based on an existing project" or specify all settings. Includes both for people to compare. DO NOT MERGE AS IS.

roligugus · 2020-06-23T00:36:10Z

sample_formatted_code.do_not_merge.c

@@ -0,0 +1,562 @@
+// Formatted using


Yes, I know these are C++ style comments. This is not meant to be committed, imho. I can always fix the commend style up if some form of this should be committed.

roligugus · 2020-06-23T00:37:00Z

sample_formatted_code.do_not_merge.c

+
+//--- Line Length ---
+// https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Coding_Style#Line-length
+// ColumnLimit: 80


ColumnLimit is the config that impacts the formatting here. I typically mentioned most of the important configs throughout the file.

I'm in favor of looking at relaxing the 80 chars. I think we've so far called this a soft limit and ~100 a hard limit. I'm okay with wider, where the main consideration should probably be the width of the review layout of github and gitlab.

For what it's worth, Linux recently switched over to 100 chars.

Anyhow, clang-format only has a hard limit. I.e. whatever the number, that'll be the limit. Downside of making it bigger than the mostly used 80 chars is that it'll reformat to 100 chars some of the code that exceeded 80 chars.

roligugus · 2020-06-23T00:48:02Z

sample_formatted_code.do_not_merge.c

+struct bla {
+    // AlignConsecutiveDeclarations
+    // AlignTrailingComments
+    int a;       /* comment */


Some of the code aligns the struct fields, some not. Which one is preferred.

Small sidenote: there's a bug/feature in clang-format that it does not correctly indent the * if pointer is right-aligned and fields are aligned,
E.g.

struct bla { int a; /* comment */ unsigned bb; /* comment */ int * ccc; /* comment */ };

I think I prefer the non-aligned for members, but aligned for the comments. But not a strong opinion.

roligugus · 2020-06-23T00:49:40Z

sample_formatted_code.do_not_merge.c

+// pointers
+// DerivePointerAlignment
+// PointerAlignment
+void *ptr;


I saw mostly right-aligned pointers so that's what I set up.

DerivePointerAlignment could be useful for whole-file formatting to "follow the prevalent left/centre/right aligned style". I disabled that.

yeah right aligned it is

roligugus · 2020-06-23T00:52:22Z

sample_formatted_code.do_not_merge.c

+
+struct Bitfields {
+    // AlignConsecutiveBitFields
+    int aaaa : 1;


There will be spaces around the colon for bitfields which is different than used in e.g. app-layer-dnp3-objects.h

Is the comment // AlignConsecutiveBitFields required here? What would happen if left off?

No, sorry. It's my poor way of tracking that "this is the clang-format setting that impacts this". None of the // comments are required, just FYI. These settings are only set in the .clang-format file.

I am currently updating the code_style.rst where it should be easier to follow.

I was trying to avoid including a "this vs that" comparison for each setting, but it probably would be helpful for the bikeshedding discussion on the format. ;)

AlignConsecutiveBitFields (bool)
If true, aligns consecutive bitfield members.

This will align the bitfield separators of consecutive lines. This will result in formattings like

int aaaa : 1; int b : 12; int ccc : 8;

Oh, this is only supported as of clang 11 which is not yet out. I don't think this should be set at all then. Explains why you won't find this setting in clang-format.full.do_not_merge. I've used clang 9 which is recent enough yet old enough to be supported by different dists.

roligugus · 2020-06-23T00:56:01Z

sample_formatted_code.do_not_merge.c

+//--- comment wrapping ---
+void multi_line_comments()
+{
+    // ReflowComments: false does not trim comments to ColumnLimit chars


I disabled limiting comments to 80 chars in the config. Hence, clang-format does not reformat comments.

While it's great to use in my experience, changing an existing code base can be messy. It requires manual fixing up of comments as we devs suck at formatting comments to a length.

Having said that, it probably would be great to enable it so that new code could take advantage of it.

I really like it when an IDE reflows my comments to 80 chars. So I'd like to see this on by default, if it doesn't get in the way too often.

catenacyber · 2020-06-23T06:37:49Z

Thanks for this proposition.
Is there a way that CI can check that Pull Request's commits are properly formatted ? Running git clang-format on them ?

roligugus · 2020-06-23T16:25:57Z

Thanks for this proposition.

Cool, just learnt that there's a draft in github.

Is there a way that CI can check that Pull Request's commits are properly formatted ? Running git clang-format on them ?

Don't know github actions myself, but there should be a way.
Initial idea would be to add a stage similar to the fuzz and cocci builds and have it fail if formatting is wrong, i.e. git clang-format does not produce the code in the same format. Have not had time to look into it. Will play with it.

We could probably add a new make target as well that could be used in the github action.

From what I can tell, these are the different potential steps:

Create the clang-format config and update code style. I am currently updating the devguide and will add to this PR once done.
Enforce formatting for new pull requests, or have at least some warnings. I would not recommend to auto-format it through CI, but rather force the dev do the leg work. Anyhow, that work lends itself to the CI infra, i.e. github actions.
We could probably provide a git pre-commit hook for people to do that. Not a big fan personally of git hooks as they are optional and some projects go crazy over it, but it could be one possible option.
Reformat all code if there is a willingness. That will need a proper strategy and multiple iterations to prep the code as well.

Reformatting all code shall be out-of-scope for this PR.

victorjulien · 2020-06-26T08:59:56Z

Thanks Roland, this is great work. The git clang-format trick looks interesting. As you can imagine my biggest worry is a massive reformat. The result of that would be nice, but what it will do to our git history... (and think of the painful rebases for anyone still sitting on unmerged branches).

roligugus · 2020-07-02T20:30:49Z

Rebases of unmerged branches can be handled with git filter-branch. I will add some make targets to help with that use case.

I fully understand the worries about massive reformatting. Been there, done that. ;)

No easy solution, really. For git bisect driven workflows, we clang-format the old version as well and then do diffs that way in case one version is formatted, and one not. Not ideal, but workable. The pain is if you quickly want to diff across the "reformatting boundary". Pre-reformat and post-reformat diffs are the albeit somewhat mundane solution. Key is to have "pure formatting commits" to indicate these boundaries.

victorjulien · 2020-07-13T15:24:04Z

Continued in #5147

roligugus · 2020-07-15T05:29:40Z

Real continuation of this is #5186

#5147 was only a test for github actions.

roligugus requested a review from a team as a code owner June 23, 2020 00:26

roligugus commented Jun 23, 2020

View reviewed changes

catenacyber marked this pull request as draft June 23, 2020 06:35

roligugus mentioned this pull request Jul 8, 2020

Clang format proposal v3 - github actions testing - please ignore #5147

Closed

victorjulien closed this Jul 13, 2020

roligugus mentioned this pull request Jul 15, 2020

Clang format proposal v4 #5186

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add clang-format file #5092

Add clang-format file #5092

roligugus commented Jun 23, 2020 •

edited

Loading

roligugus Jun 23, 2020

roligugus Jun 23, 2020 •

edited

Loading

victorjulien Jun 26, 2020

roligugus Jul 2, 2020

roligugus Jun 23, 2020

victorjulien Jun 26, 2020

roligugus Jun 23, 2020

victorjulien Jun 26, 2020

roligugus Jun 23, 2020

jasonish Jun 23, 2020

roligugus Jun 23, 2020

roligugus Jun 23, 2020

roligugus Jun 23, 2020

jasonish Jun 23, 2020

catenacyber commented Jun 23, 2020

roligugus commented Jun 23, 2020

victorjulien commented Jun 26, 2020

roligugus commented Jul 2, 2020 •

edited

Loading

victorjulien commented Jul 13, 2020

roligugus commented Jul 15, 2020

Add clang-format file #5092

Add clang-format file #5092

Conversation

roligugus commented Jun 23, 2020 • edited Loading

Describe changes

Discussion and decisions

Requires

How to use it?

Other Notes

Choose a reason for hiding this comment

roligugus Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catenacyber commented Jun 23, 2020

roligugus commented Jun 23, 2020

victorjulien commented Jun 26, 2020

roligugus commented Jul 2, 2020 • edited Loading

victorjulien commented Jul 13, 2020

roligugus commented Jul 15, 2020

roligugus commented Jun 23, 2020 •

edited

Loading

roligugus Jun 23, 2020 •

edited

Loading

roligugus commented Jul 2, 2020 •

edited

Loading