llext: Use clang format on all llext files #93770
Conversation
teburd
added
the
Trivial
subsys/llext/llext_link.c
Outdated
| shdr->sh_info, | ||
| &rel); | ||
| uintptr_t op_loc = | ||
| llext_get_reloc_instruction_location(ldr, ext, shdr->sh_info, &rel); |
There was a problem hiding this comment.
on the one hand automation is good, and having rules helps avoid mostly needless discussions... OTOH... do most of us find this a good change? There was another similar split of x = a - b in lines 330-331 in the same file above...
There was a problem hiding this comment.
So we can get the formatter to adjust where the line break occurs with some options, I think given this style we want to penalize breaking no the assignment and after the first break.
We have the formatter undoing stuff like this else where
subsys/llext/llext_load.c
Outdated
| size_t top_ofs = MAX(region->sh_offset + region->sh_size, | ||
| shdr->sh_offset + shdr->sh_size); | ||
| size_t top_ofs = | ||
| MAX(region->sh_offset + region->sh_size, shdr->sh_offset + shdr->sh_size); |
subsys/llext/llext_load.c
Outdated
| exp_tab->sym_cnt = ldr->sects[LLEXT_MEM_EXPORT].sh_size | ||
| / sizeof(struct llext_symbol); | ||
| exp_tab->sym_cnt = | ||
| ldr->sects[LLEXT_MEM_EXPORT].sh_size / sizeof(struct llext_symbol); |
There was a problem hiding this comment.
maybe there's a parameter to tell it to not split lines after an assignment
There was a problem hiding this comment.
The project has a clang format config file, I’ll see if there’s an option for this
There was a problem hiding this comment.
There was a fix for this awhile ago, need to see which option adjusts it https://reviews.llvm.org/D32477
There was a problem hiding this comment.
clang-format is a great tool to enforce a minimum standard and most of the changes here are great, but personally I'm against requiring absolute compliance: I have highlighted several places where word wraps were chosen to maximize immediate pattern recognition; with the "cleanup" these are now more difficult to spot.
So soft -1 (to the rule), but ofc I’m willing to go with whatever is ultimately decided.
| LOG_DBG("relocation section %s (%d) acting on section %d has %zd relocations", name, | ||
| i, shdr->sh_info, (size_t)rel_cnt); |
There was a problem hiding this comment.
printf lines with only the format on the first line, arguments on next ones
There was a problem hiding this comment.
let me see if there's a clang-format option for this as well
| if (!ldr->sects[LLEXT_MEM_SHSTRTAB].sh_type || !ldr->sects[LLEXT_MEM_STRTAB].sh_type || | ||
| !ldr->sects[LLEXT_MEM_SYMTAB].sh_type) { |
| if (x->sh_type == SHT_NULL || x->sh_size == 0 || y->sh_type == SHT_NULL || | ||
| y->sh_size == 0) { |
There was a problem hiding this comment.
again, optimized but less readable
| SHELL_HELP("Load an elf file directly from filesystem.", "<ext_name> " \ | ||
| "<ext_llext_file_name>") |
There was a problem hiding this comment.
split a string in the middle... resulting in a longer line
| i, REGION_BOT(x, sh_offset), REGION_TOP(x, sh_offset), j, | ||
| REGION_BOT(y, sh_offset), REGION_TOP(y, sh_offset)); |
teburd
force-pushed
the
llext_clang_format
branch
from
36a7c88 to
7ea5a19
Compare
zephyrbot
requested review from
carlescufi,
jfischer-no,
keith-zephyr,
nashif and
simhein
|
Ultimately most of the format changes here are quite nice to me. It's a thoughtless excercise to run clang-format and avoids formatting nits which I really never like seeing (I want to see quality structural review comments). But if the tool can't be made with its many options to get us like 95% success with a few whiffs (the tool can always be disabled for small sections like logically aligned printfs) then its just not good enough still which is disappointing. I'll tinker with the rules a bit more today to try and address the comments from @pillo79 and @lyakh (I appreciate you two looking this over!). |
These options tell clang-format penalize the formatter for trying to break a long line up on particular elements like assignment, opening brackets, and such. Adjusting these means long lines are broken at more natural places that match existing code. Signed-off-by: Tom Burdick <[email protected]>
If we stick with clang-format we don't need to nitpick over little spacing issues for new contributions, we can simply ask people to run the formatting tool. Much simpler. Signed-off-by: Tom Burdick <[email protected]>
teburd
force-pushed
the
llext_clang_format
branch
from
7ea5a19 to
5eb8787
Compare
|
If we stick with clang-format we don't need to nitpick over little spacing issues for new contributions, we can simply ask people to run the formatting tool. Much simpler.
Noticed these files weren't formatted with the tool in #83423 and the result is nits over spacing which are not the kind of review comments I want to see, use the tool to ensure spacing/formatting nits are unnecessary.