clang-format #91

lucasdemarchi · 2024-09-02T05:09:42Z

Fix some details as commented in #88 and run clang-format. The github-action integration is not here yet.

The double ';;' were clearly a mistake and clang-format only makes it worse if we try running it. Remove the mistake before mass-converting the source code. Signed-off-by: Lucas De Marchi <[email protected]>

Import .clang-format from Linux kernel as of v6.11-rc6. Signed-off-by: Lucas De Marchi <[email protected]>

- Define our own foreach macros - Add defines for attributes. This also needs the minimum clang-format version to be raised so it has the AttributeMacros setting. - Redefine a few settings related to max number of columns and penalties for breaking lines Signed-off-by: Lucas De Marchi <[email protected]>

Run clang-format Signed-off-by: Lucas De Marchi <[email protected]>

evelikov

Generally I wouldn't be too worried that declaration/definitions funcname start at first column. It's very common in mesa, drm and starting to pick-up in some parts of the kernel.

You can "git grep -1 ^some_function", to find the origin (with the type). No need to setup indexing tools (scope?) or look through hundreds of matches. It just works everywhere.

Edit: another bonus is that we could have

# this
int _print_format_(...) _must_check_ _another_one_
function_name(very_long_arg1, very_long_arg2, very_long_arg3);

# instead of this
int _print_format_(...) _must_check_ _another_one_ function_name(very_long_arg1,
                                                                 very_long_arg2,
                                                                 very_long_arg3);

At a glance it looks fine, but I would kindly request that we:

adding a trailing comma for the last entry, so clang-format doesn't go crazy
if ^^ doesn't help, personally I'm leaning to clang-format off the block
split the patch per-folder, since it's massive for one-go
last but not least, can we let this cook until my docs rework series is merged... will open later tonight and this will cause lots of conflicts

There a lots of "aside" aka orthogonal questions, that I'm mildly curious but are non-blocking

Also thanks for kicking this off.

evelikov · 2024-09-02T12:53:03Z

.clang-format

@@ -52,7 +52,7 @@ BreakConstructorInitializersBeforeComma: false
 BreakConstructorInitializers: BeforeComma
 BreakAfterJavaFieldAnnotations: false
 BreakStringLiterals: false
-ColumnLimit: 80
+ColumnLimit: 90


Oh noooo, I can no longer have two panes side-by-side :-P

Semi-joking aside (I do use ^^) - if we do this, the .editorconfig also needs an update.

I hope your pane is modern enough in 2024 to have horizontal scroll for the eventual lines going over 80 :). What I find annoying with auto format is that we don't have a good option for "prefer 80, but don't bother breaking it up if it's slightly over" - IMO we should be able to define soft and hard limits for autoformat tools. I think the Penalty* settings in clang get close, but I still see it doing some horrible changes because of column limit

2x 80 (with a few extra symbols) just happens to be comfortably sized on my 13inch laptop. Any other device or single (tmux) pane is fine. Hope it was clear I was joking

Cannot agree more on the hard vs soft limits. If we keep different values in .clang-format vs .editorconfig, clang-format is likely to repeatedly re-format my patches ;-)

evelikov · 2024-09-02T12:57:22Z

.clang-format

+  - _must_check_
+  - _printf_format_
+  - _unused_
+  - noreturn


Note for another day: the above could use some cleanup.

Huge thanks for finding this trick o/

The cleanup was incorporated in #114

evelikov · 2024-09-02T13:07:12Z

libkmod/libkmod-file.c

+		     kmod_file_load_xz },
+		   { sizeof(magic_zlib), KMOD_FILE_COMPRESSION_ZLIB, magic_zlib,
+		     kmod_file_load_zlib },
+		   { 0, KMOD_FILE_COMPRESSION_NONE, NULL, load_reg } };


The new formatting looks bizzaire IMHO.

It's usually a case of missing a trailing comma after the last element (allowed with C99), which trips clang-format to try and reshuffle this on single line. In some instances, that's not enough and we're better off with clang-format off the respective hunk IMHO.

Any chance we can add the missing the spaces around curly brackets and clang-format off the hunk?

I don't have a preference on the alignment.

Trailing commas added in #114

evelikov · 2024-09-02T13:10:47Z

libkmod/libkmod-internal.h

+struct kmod_list *kmod_list_insert_before(struct kmod_list *list, const void *data)
+	__attribute__((nonnull(2)));
+struct kmod_list *kmod_list_append_list(struct kmod_list *list1, struct kmod_list *list2)
+	_must_check_;


If the lhs suggestion above doesn't work, clang-format off the hunk seems more suitable IMHO.

what's "lhs suggestion"?

// original struct kmod_list *kmod_list_append_list(struct kmod_list *list1, struct kmod_list *list2) _must_check_; // attribute on the lhs of function name struct _must_check_ kmod_list *kmod_list_append_list(struct kmod_list *list1, struct kmod_list *list2);

I have seen the latter style a lot more than the former. Also MSVC errors with the original ... not that it matters here.

ahh... lhs == left hand side?

Linux kernel has coding style guideline for that, although I don't really like it much since it changes the order between declaration and definition (but afair it's exactly because the syntax would otherwise not be accepted by compilers). See https://www.kernel.org/doc/html/latest/process/coding-style.html#function-prototypes

Added some extra macros for nonnull and moved to the left hand-side. Note it's slightly different than the kernel guidance yet consistent with upcoming C23 standard. See #114

evelikov · 2024-09-02T14:31:19Z

libkmod/libkmod-module.c

+	if ((mod->alias != NULL &&
+	     ((err = flags & KMOD_PROBE_APPLY_BLACKLIST_ALIAS_ONLY))) ||
+	    (err = flags & KMOD_PROBE_APPLY_BLACKLIST_ALL) ||
+	    (err = flags & KMOD_PROBE_APPLY_BLACKLIST)) {


Aside: do you have a strong preference to keeping assignments in the if statement or is this an exception?

Opened #112

evelikov · 2024-09-02T14:59:18Z

testsuite/test-modinfo.c

-		progname, "-F", "filename",
-		"mod-simple",
-		NULL,
+		progname, "-F", "filename", "mod-simple", NULL,
 	};


Here and below: the new formatting makes the args wildly different instances. It'll be great if they're consistent, might need to clang-format off for that though. Not a huge deal either way.

Perhaps:

progname, list, of, args, modulename, NULL,

evelikov · 2024-09-02T15:01:20Z

testsuite/test-util.c

+			{ strdup("-aa-[bb]-cc-"), "_aa_[bb]_cc_" },
+			{ strdup("-aa-[b-b]-cc-"), "_aa_[b-b]_cc_" },
+			{ strdup("-aa-b[-]b-cc"), "_aa_b[-]b_cc" },
+			{} },


Another case of trailing comma and/or clang-format off?

evelikov · 2024-09-02T15:06:38Z

libkmod/libkmod-index.c

-			    (sizeof(uint32_t) * child_count)) % sizeof(void *);
+	children_padding =
+		(sizeof(struct index_mm_node) + (sizeof(uint32_t) * child_count)) %
+		sizeof(void *);


Aside: I think it'll be great if we check and switch any outstanding zero/one sized (trailing arrays) and add some handy wrappers to avoid doing this kind of maths by hand.

evelikov · 2024-09-02T15:08:19Z

libkmod/libkmod-internal.h

-		const char *format, ...) __attribute__((format(printf, 6, 7))) __attribute__((nonnull(1, 3, 5)));
+void kmod_log(const struct kmod_ctx *ctx, int priority, const char *file, int line,
+	      const char *fn, const char *format, ...)
+	__attribute__((format(printf, 6, 7))) __attribute__((nonnull(1, 3, 5)));


Generally, I think it'll be better if we move the attrib decorations on function lhs. As result the clang-format is producing much more sensible output... Also it's the more common (afaict) format in the kernel.

evelikov · 2024-09-02T15:18:23Z

testsuite/test-modinfo.c

-			    "/mod-simple-sha1.ko", \
-			    "/mod-simple-sha256.ko",	\
+#define DEFINE_MODINFO_SIGN_TEST(_field)                                              \
+	DEFINE_MODINFO_TEST(_field, , "/mod-simple-sha1.ko", "/mod-simple-sha256.ko", \
 			    "/mod-simple-pkcs7.ko")


The trailing comma can help with these two instances... Although the new format doesn't look too bad either.

lucasdemarchi · 2024-09-03T13:33:50Z

You can "git grep -1 ^some_function", to find the origin (with the type). No need to setup indexing tools (scope?) or look through hundreds of matches. It just works everywhere.

$ git grep -1 "array_realloc"
shared/array.c-
shared/array.c:static int array_realloc(struct array *array, size_t new_total)
shared/array.c-{
--
shared/array.c-         /* ignore error */
shared/array.c:         array_realloc(array, array->total - array->step);
shared/array.c- }
--
shared/array.c-                 return -ENOMEM;
shared/array.c:         r = array_realloc(array, array->total + array->step);
shared/array.c-         if (r < 0)

vs

$ git grep "array_realloc"
shared/array.c:static int array_realloc(struct array *array, size_t new_total)
shared/array.c:         array_realloc(array, array->total - array->step);
shared/array.c:         r = array_realloc(array, array->total + array->step);

For me there's a clear winner on better approach here if someone is grepping.

evelikov · 2024-09-03T14:01:36Z

As you can see below, the benefits become apparent if looking for function with more than a handful of callers.

$ git grep -w kmod_list_append
libkmod/libkmod-config.c:	l = kmod_list_append(*list, cmd);
libkmod/libkmod-config.c:	list = kmod_list_append(config->options, opt);
libkmod/libkmod-config.c:	list = kmod_list_append(config->aliases, alias);
libkmod/libkmod-config.c:	list = kmod_list_append(config->blacklists, p);
libkmod/libkmod-config.c:	list = kmod_list_append(config->softdeps, dep);
libkmod/libkmod-config.c:	list = kmod_list_append(config->weakdeps, dep);
libkmod/libkmod-config.c:		tmp = kmod_list_append(*list, cf);
libkmod/libkmod-config.c:		tmp = kmod_list_append(path_list, cf);
libkmod/libkmod-internal.h:struct kmod_list *kmod_list_append(struct kmod_list *list, const void *data) _must_check_ __attribute__((nonnull(2)));
libkmod/libkmod-list.c:struct kmod_list *kmod_list_append(struct kmod_list *list, const void *data)
libkmod/libkmod-list.c:		return kmod_list_append(list, data);
libkmod/libkmod-list.c:		return kmod_list_append(list, data);
libkmod/libkmod-module.c:		l_new = kmod_list_append(list_new, kmod_module_ref(l->data));
libkmod/libkmod-module.c:		node = kmod_list_append(*output, mod);
libkmod/libkmod-module.c:	l = kmod_list_append(*list, kmod_module_ref(mod));
libkmod/libkmod-module.c:		l = kmod_list_append(*list, kmod_module_ref(mod));
libkmod/libkmod-module.c:		node = kmod_list_append(l, m);
libkmod/libkmod-module.c:		l = kmod_list_append(list, holder);
libkmod/libkmod-module.c:		l = kmod_list_append(list, section);
libkmod/libkmod-module.c:	n = kmod_list_append(*list, info);
libkmod/libkmod-module.c:		n = kmod_list_append(*list, mv);
libkmod/libkmod-module.c:		n = kmod_list_append(*list, mv);
libkmod/libkmod-module.c:		n = kmod_list_append(*list, mv);
libkmod/libkmod.c:		*list = kmod_list_append(*list, mod);
libkmod/libkmod.c:		*list = kmod_list_append(*list, mod);
libkmod/libkmod.c:		*list = kmod_list_append(*list, mod);
libkmod/libkmod.c:			*list = kmod_list_append(*list, mod);
libkmod/libkmod.c:			node = kmod_list_append(*list, mod);
libkmod/libkmod.c:			node = kmod_list_append(*list, mod);
testsuite/test-blacklist.c:		list = kmod_list_append(list, mod);
testsuite/test-list.c:		list = kmod_list_append(list, v[i]);
testsuite/test-list.c:		list = kmod_list_append(list, v[i]);
testsuite/test-list.c:		list = kmod_list_append(list, v[i]);
testsuite/test-list.c:		list = kmod_list_append(list, v[i]);
testsuite/test-list.c:		a = kmod_list_append(a, v[i]);
testsuite/test-list.c:		b = kmod_list_append(b, v[i]);
tools/depmod.c:	l = kmod_list_append(free_list, root);
tools/depmod.c:			l = kmod_list_append(free_list, v);
tools/depmod.c:		l = kmod_list_append(roots, m);

$ git grep -w1 ^kmod_list_append
libkmod/libkmod-internal.h-struct kmod_list *
libkmod/libkmod-internal.h:kmod_list_append(struct kmod_list *list, const void *data) _must_check_ __attribute__((nonnull(2)));
libkmod/libkmod-internal.h-struct kmod_list *kmod_list_prepend(struct kmod_list *list, const void *data) _must_check_ __attribute__((nonnull(2)));
--
libkmod/libkmod-list.c-struct kmod_list *
libkmod/libkmod-list.c:kmod_list_append(struct kmod_list *list, const void *data)
libkmod/libkmod-list.c-{

lucasdemarchi · 2024-09-03T14:44:31Z

Then you just miss the definitions/declarations that have the type in the same line. Which means for every function you grep you have to try 2 greps, with and without ^. Since I use vim, I just do :GGrep <C-r><C-w> and expect it to give me reasonable results when I'm navigating the code.

Make the grep simple by not having a 50%-50% distribution on how we declare/define functions based on line length.

evelikov · 2024-09-04T14:11:34Z

Since my initial comment was sparse, let me erro on the verbose side and try again.

Click to see wall of text 🤣

The premise is that we can enforce the definitions/declarations (the function name that is)t to start on newline - there is a clang toggle for that. This means that finding them is one extra character ^. Doing so not require any IDE, editor or custom tools - scope, vim, vim-fugitive (vim user here but never installed it), emacs, vscode, etc. It just works with "anything" - grep, git grep, ripgrep, sed, perl, at least.

Without this, currently one is presented with potentially massive list and having to look through.

If the function signature (return value and/or arguments) is of interest, one would regularly need context (-5 my go-to), especially if they have multiple function arguments spanning across... Admittedly most kmod instances are nice and short.

After chatting with people - both software engineers and not - many have went to defaults, after years of carefully curated setups. For some of them didn't like the "constant faff" of maintaining, setting up new machine, etc. Others saw "the best editor" topic alienating. But more importantly doing demo debug session, in a custom setups, makes things harder for the audience.

I can somewhat agree with those arguments, even though I have a slightly tweaked vim setup.

Hope that makes things clearer. If not, I wouldn't worry about it.

As a closing though: It's fine if my suggestion doesn't seem appealing, the topic is quite subjective anyway.

lucasdemarchi · 2024-09-04T16:50:00Z

This means that finding them is one extra character ^.

My problem with it is the badly in "it just works, but badly". Or... it works, but produces utter garbage when all I wanted was the single line. Because most of the time I am grepping by the function, I'm not specifically looking for the context. When I'm interested in the context I just jump to that. So this makes the main use case look bad.

Without this, currently one is presented with potentially massive list and having to look through.

and it usually takes less time to visually filter out the one with a type in the same line than it took to type the whole thing.

Hope that makes things clearer. If not, I wouldn't worry about it.

It does, but I guess the final opinion we 2 have on the matter is heavily influenced by the experience we bring from other projects. I'm not willing to change clang-format in kmod to force the definition/declaration in a new line because: a) it doesn't match 90% of the current ones;
b) I can't rely on grep -1 ^func for my day to day coding because while it works if you convert the entire project (with an awful output as complained above), I'm sure the other 4 projects I regularly contribute to won't care about changing that. I'm more comfortable with the simple grep that works 90% of the time in all projects than having this.

As a closing though: It's fine if my suggestion doesn't seem appealing, the topic is quite subjective anyway.

I appreciate your explanation, but as you said this is subjective and unlikely to have consensus. Let's leave this as is and change the other parts where we agree with. Thanks

lucasdemarchi · 2024-09-11T04:39:48Z

Superseded by #118

lucasdemarchi added 4 commits September 1, 2024 22:49

Drop empty statements

2a51278

The double ';;' were clearly a mistake and clang-format only makes it worse if we try running it. Remove the mistake before mass-converting the source code. Signed-off-by: Lucas De Marchi <[email protected]>

clang-format: Import from kernel

3a0a2d8

Import .clang-format from Linux kernel as of v6.11-rc6. Signed-off-by: Lucas De Marchi <[email protected]>

Mass-convert with clang-format

2b00711

Run clang-format Signed-off-by: Lucas De Marchi <[email protected]>

lucasdemarchi requested a review from evelikov September 2, 2024 05:09

evelikov requested changes Sep 2, 2024

View reviewed changes

This was referenced Sep 7, 2024

Some clang-format inspired cleanups #114

Closed

Prep and enable clang-format #118

Closed

lucasdemarchi closed this Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clang-format #91

clang-format #91

lucasdemarchi commented Sep 2, 2024

evelikov left a comment •

edited

Loading

evelikov Sep 2, 2024

lucasdemarchi Sep 3, 2024

evelikov Sep 4, 2024

evelikov Sep 2, 2024

evelikov Sep 7, 2024

evelikov Sep 2, 2024

evelikov Sep 7, 2024

evelikov Sep 2, 2024

lucasdemarchi Sep 3, 2024 •

edited

Loading

evelikov Sep 4, 2024 •

edited

Loading

lucasdemarchi Sep 4, 2024

evelikov Sep 7, 2024

evelikov Sep 2, 2024

evelikov Sep 6, 2024

evelikov Sep 2, 2024

evelikov Sep 2, 2024

evelikov Sep 2, 2024

evelikov Sep 2, 2024

evelikov Sep 2, 2024

lucasdemarchi commented Sep 3, 2024

evelikov commented Sep 3, 2024

lucasdemarchi commented Sep 3, 2024

evelikov commented Sep 4, 2024

lucasdemarchi commented Sep 4, 2024

lucasdemarchi commented Sep 11, 2024

clang-format #91

clang-format #91

Conversation

lucasdemarchi commented Sep 2, 2024

evelikov left a comment • 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

lucasdemarchi Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

evelikov Sep 4, 2024 • 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

lucasdemarchi commented Sep 3, 2024

evelikov commented Sep 3, 2024

lucasdemarchi commented Sep 3, 2024

evelikov commented Sep 4, 2024

lucasdemarchi commented Sep 4, 2024

lucasdemarchi commented Sep 11, 2024

evelikov left a comment •

edited

Loading

lucasdemarchi Sep 3, 2024 •

edited

Loading

evelikov Sep 4, 2024 •

edited

Loading