Clang format custom style #1471
Conversation
javorszky
force-pushed
the
gabor/clang-format-custom
branch
5 times, most recently
from
ad63c79
to
e22a773
Compare
|
I'm using Clang 18.1.8 on Fedora 40 and this will need to work on older versions than that. Currently I'm hitting Case Arrows? I've never even heard of such a thing! Let's see what this actually does Hmm, that ain't C, hell, that ain't even C++. AFAICT, it's Java! So we can happily scrub that... |
|
With some tweaks, this could be acceptable... The following are code snippets showing the current code on the left and new code on the right (From src/wasm/nxt_rt_wasmtime.c) struct nxt_wasmtime_ctx_s { | struct nxt_wasmtime_ctx_s {
wasm_engine_t *engine; | wasm_engine_t *engine;
wasmtime_store_t *store; | wasmtime_store_t *store;
wasmtime_memory_t memory; | wasmtime_memory_t memory;
wasmtime_module_t *module; | wasmtime_module_t *module;
wasmtime_linker_t *linker; | wasmtime_linker_t *linker;
wasmtime_context_t *ctx; | wasmtime_context_t *ctx;
}; | }Aligning pointer variables differently is a nginx thing that Unit doesn't do, let's not start... static int | static int
nxt_wasmtime_init(nxt_wasm_ctx_t *ctx) | nxt_wasmtime_init(nxt_wasm_ctx_t *ctx)
{ | {
int err; | int err;
FILE *fp; | FILE *fp;
size_t file_size; | size_t file_size;
wasm_byte_vec_t wasm; | wasm_byte_vec_t wasm;
wasmtime_error_t *error; | wasmtime_error_t *error;
nxt_wasmtime_ctx_t *rt_ctx = &nxt_wasmtime_ctx; | nxt_wasmtime_ctx_t *rt_ctx = &nxt_wasmtime_ctx;
|
rt_ctx->engine = wasm_engine_new(); | rt_ctx->engine = wasm_engine_new();
rt_ctx->store = wasmtime_store_new(rt_ctx->engine, NULL, NULL); | rt_ctx->store = wasmtime_store_new(rt_ctx->engine, NULL, NULL);
rt_ctx->ctx = wasmtime_store_context(rt_ctx->store); | rt_ctx->ctx = wasmtime_store_context(rt_ctx->store);
|
rt_ctx->linker = wasmtime_linker_new(rt_ctx->engine); | rt_ctx->linker = wasmtime_linker_new(rt_ctx->engine);
error = wasmtime_linker_define_wasi(rt_ctx->linker); | error = wasmtime_linker_define_wasi(rt_ctx->linker); Let's disable this assignment alignment thing. Sometimes aligning things can make sense, but this general thing in functions like this is I think generally a bad idea, apart from not looking great, it could lead to lots of unnecessary code churn if you make a single line change but then have to re-align a whole bunch of stuff, not good. static const struct { | static const struct {
const char *func_name; | const char *func_name;
|
wasmtime_func_callback_t func; | wasmtime_func_callback_t func;
wasm_valkind_t params[1]; | wasm_valkind_t params[1];
wasm_valkind_t results[1]; | wasm_valkind_t results[1];
|
enum { | enum {
NXT_WASM_FT_0_0, | NXT_WASM_FT_0_0,
NXT_WASM_FT_1_0, | NXT_WASM_FT_1_0,
NXT_WASM_FT_0_1, | NXT_WASM_FT_0_1,
} ft; | } ft;
} import_functions[] = { | } import_functions[] = {{.func_name = "nxt_wasm_get_init_mem_size",
{ | .func = nxt_wasm_get_init_mem_size,
.func_name = "nxt_wasm_get_init_mem_size", | .results = {WASM_I32},
.func = nxt_wasm_get_init_mem_size, | .ft = NXT_WASM_FT_0_1},
.results = { WASM_I32 }, | {.func_name = "nxt_wasm_response_end",
.ft = NXT_WASM_FT_0_1 | .func = nxt_wasm_response_end,
}, { | .ft = NXT_WASM_FT_0_0},
.func_name = "nxt_wasm_response_end", | {.func_name = "nxt_wasm_send_response",
.func = nxt_wasm_response_end, | .func = nxt_wasm_send_response,
.ft = NXT_WASM_FT_0_0 | .params = {WASM_I32},
}, { | .ft = NXT_WASM_FT_1_0},
.func_name = "nxt_wasm_send_response", | {.func_name = "nxt_wasm_send_headers",
.func = nxt_wasm_send_response, | .func = nxt_wasm_send_headers,
.params = { WASM_I32 }, | .params = {WASM_I32},
.ft = NXT_WASM_FT_1_0 | .ft = NXT_WASM_FT_1_0},
}, { | {.func_name = "nxt_wasm_set_resp_status",
.func_name = "nxt_wasm_send_headers", | .func = nxt_wasm_set_resp_status,
.func = nxt_wasm_send_headers, | .params = {WASM_I32},
.params = { WASM_I32 }, | .ft = NXT_WASM_FT_1_0},
.ft = NXT_WASM_FT_1_0 |
}, { | {}},
.func_name = "nxt_wasm_set_resp_status", | *imf;
.func = nxt_wasm_set_resp_status, | -----------------------------------------------------------------------------------
.params = { WASM_I32 }, | -----------------------------------------------------------------------------------
.ft = NXT_WASM_FT_1_0 | -----------------------------------------------------------------------------------
}, | -----------------------------------------------------------------------------------
| -----------------------------------------------------------------------------------
{ } | -----------------------------------------------------------------------------------
}, *imf; | -----------------------------------------------------------------------------------The definition changes are OK, however the declaration part of the structure array is not great and we have a lot of this kind of thing. We need to get it to basically leave the } import_functions[] = {
...
}, *imf; part alone. From (src/wasm/nxt_wasm.h) extern void nxt_wasm_do_response_end(nxt_wasm_ctx_t *ctx); | extern void
extern void nxt_wasm_do_send_response(nxt_wasm_ctx_t *ctx, uint32_t offset); | nxt_wasm_do_response_end(nxt_wasm_ctx_t *ctx);
extern void nxt_wasm_do_send_headers(nxt_wasm_ctx_t *ctx, uint32_t offset); | extern void
| nxt_wasm_do_send_response(nxt_wasm_ctx_t *ctx, uint32_t offset);
#endif /* _NXT_WASM_H_INCLUDED_ */ | extern void
------------------------------------------------------------------------------------| nxt_wasm_do_send_headers(nxt_wasm_ctx_t *ctx, uint32_t offset);
------------------------------------------------------------------------------------| I know we do this with function definitions (put the return type on its own line) but we don't do that for function declarations and if anything I'd prefer to see us stop doing it with function definitions. It doesn't look too bad above but when you have a lot of these, it won't look good... Only some files seems to have had this change, for example, src/nxt_controller.c hasn't (though it has had other changes). In fact this patch set seems to have only changed some files and even only some changes to some files. Something weird with this... Oh wow!, the following is a dangerous change! WTF!!! (From src/nxt_main_process.c) static const nxt_str_t root_path = nxt_string("/"); | static nxt_str_t root_path = nxt_string("/");
static const nxt_str_t mounts_name = nxt_string("mounts"); | static nxt_str_t mounts_name = nxt_string("mounts"); We've lost the I have no idea what the thought process is here... lets look at at a wee example sc.c #include <stdio.h>
int main(void)
{
static char *s = "literal";
printf("%s\n", s);
s[0] = '\0';
/* Should never reach (segfault above) */
printf("%s\n", s);
return 0;
}If you compile and run that, what do you think happens? (the comment gives it away)... Technically the behaviour is undefined, however a segmentation fault is the likeliest outcome... What difference does adding the To the generated code? None (problematic line commented out) However if we try to compile this #include <stdio.h>
int main(void)
{
static const char *s = "literal";
printf("%s\n", s);
s[0] = '\0';
/* Should never reach (segfault above) */
printf("%s\n", s);
return 0;
}So we need to disable this brain damage! This is why this sort of code wide change needs very close scrutiny and can't be rushed... Can't really make more comments until I see this formatting change applied more widely... One thing I would very much like to see is no spaces between casts and the castee. We currently have a mixture of this in the code and it's something I've previously left to individual preference. I.e. I'd like to propose |
Yeah, there are a bunch of rules that aren't C specific. Where the documentation makes this available I added a I appreciate you double checking these! |
For CI I think we can find version 19 to work. For local development if someone doesn't have version 19, or can't have version 19 due to OS version limitations, I plan on creating a small docker container that would have version 19 and run that, so we don't need to support a format file that's old. |
|
re: clang-format versions, I don't think we need anything that was added in 19; I'm fine standardizing on 18 for now. No need to support earlier; clang-format is an optional development dependency; the CI can check and output a diff if changes need to be made and the developer didn't run clang-format locally. |
javorszky
force-pushed
the
gabor/clang-format-custom
branch
from
38cdc9e
to
4d9f536
Compare
|
I wrote a tool to find an ideal / least invasive current clang-format file for unit here: https://github.com/javorszky/clang-format-finder-unit |
The tool in question is https://github.com/javorszky/clang-format-finder-unit, which tried most combination of values for most options and came to an ideal format file that changes the least amount of lines. See the other repository to see how that works. This doesn't mean that this format file is in alignment with what we want, it means this is the least invasive current setting.
javorszky
force-pushed
the
gabor/clang-format-custom
branch
from
4d9f536
to
a1eb6a6
Compare
Rulesets are custom based on LLVM, internal code styling doc taken into account
See the .clang-format file for rules, and PR #1469