Clang format custom style

[javorszky]

Rulesets are custom based on LLVM, internal code styling doc taken into account

$ clang-format -i --style=file:.clang-format src/*.h src/*.c src/**/*.c src/**/*.h

See the .clang-format file for rules, and PR #1469

[ac000]

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

$ clang-format --dry-run src/wasm/nxt_rt_wasmtime.c 
/home/andrew/src/unit/.clang-format:66:3: error: unknown key 'AlignCaseArrows'
  AlignCaseArrows:  true
  ^~~~~~~~~~~~~~~
Error reading /home/andrew/src/unit/.clang-format: Invalid argument

Case Arrows? I've never even heard of such a thing!

Let's see what this actually does

true:
i = switch (day) {
  case THURSDAY, SATURDAY -> 8;
  case WEDNESDAY          -> 9;
  default                 -> 0;
};

false:
i = switch (day) {
  case THURSDAY, SATURDAY -> 8;
  case WEDNESDAY ->          9;
  default ->                 0;
};

Hmm, that ain't C, hell, that ain't even C++.

AFAICT, it's Java!

So we can happily scrub that...

[ac000]

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 const qualifiers! (after I've spent a bunch of time adding a bunch of missing ones as well).

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)...

$ make sc
cc     sc.c   -o sc
$ ./sc 
literal
Segmentation fault (core dumped)

Technically the behaviour is undefined, however a segmentation fault is the likeliest outcome...

What difference does adding the const qualifier make?

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;
}

$ make sc
cc     sc.c   -o sc
sc.c: In function ‘main’:
sc.c:8:14: error: assignment of read-only location ‘*s’
    8 |         s[0] = '\0';
      |              ^
make: *** [<builtin>: sc] Error 1

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

SpaceAfterCStyleCast: false

[javorszky]

Hmm, that ain't C, hell, that ain't even C++.

AFAICT, it's Java!

So we can happily scrub that...

Yeah, there are a bunch of rules that aren't C specific. Where the documentation makes this available I added a @NOTUSED tag to the comment for the rule.

I appreciate you double checking these!

[javorszky]

I'm using Clang 18.1.8 on Fedora 40 and this will need to work on older versions than that.

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.

[callahad]

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]

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