Bluetooth Subsystem Code Style Guidelines

[Thalley]

As per the Discord thread (https://discord.com/channels/720317445772017664/916269984529907772/916270130726572072) this RFC will try to gather information about which code style we want to follow in the Bluetooth Subsystem. We will only define the style for the Bluetooth subsystem at this point, as trying to make these guidelines/rules tree wide will take much longer time, and the different subsystem already have different style.

The goal is to create a set of code style that we can officially refer to when reviewing pull requests, as currently many reviewers have their own personal opinion where some are conflicting, making it hard for contributors to add code.

Each rule/guideline will be in their own comment, which will allow us to "vote" which rules we want the most (let's cross fingers for that to actually work, otherwise we may need to do this in a different way).

Things to consider for this:

1. The coding style may be different for the host and controller
2. The coding style should preferable be checkable by existing tools (e.g. clang-format)
3. The coding style should preferable be able to be autoformatted by existing tools
4. The coding style should not directly conflict with existing format tools
5. The coding style shall not conflict with https://docs.zephyrproject.org/latest/contribute/index.html#coding-style or https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html#coding-guidelines or duplicate anything from those

[Thalley]

Reverse Christmas list/pyramid:

Variables should be declared with the longer lines on top:

int my_int_with_initialization = 5;
struct my_struct_t my_struct; 
float my_float;
int my_int;

TBD: Should statically declared variable follow the same pattern?

[Thalley]

Declarations at top of scope:

Variables shall always be declared at top of scope

OK:

int my_int;
float my_float;

my_int = 0;

if (my_int) {
    int another_int = 5;
    ...
}

Not OK:

float my_float;

my_float = 0;

int my_int = 5;

if (my_int) {
    int another_int = 5;
    ...
}

TBD:

int another_int = 5;
int my_int;
float my_float;

my_int = 0;

if (my_int) {
    /* another_int only used in this scope */
    ...
}

Also TBD:

int my_int;
float my_float;

my_int = 0;

#if FOO
int another_int = 5;
#endif /* FOO */ 

[jhedberg]

Is the origin for assuming this rule exists some explicit statement by someone, or simply by looking at how the current code looks like? E.g. I would never reject inline declarations as long as they're placed in a natural way close to their usage. One other rule I'd add to this (which OTOH should be applicable tree-wide and not just for Bluetooth): declarations should be used in the smallest possible scope, i.e. if the usage of a variable is limited to a single sub-scope, then the declaration should move inside that sub-scope.

[Thalley]

Declaring top of scope is something I've always used for compiled languages as far as I can remember.

I wonder if it stems from OOB languages such as Java or C# where you typically declare your properties at the top before the methods?

In any case, this is not something I feel that strongly about as long as we, as you also mention, keep the scope as small as possible.

In terms of keeping the scope as small as possible, how you do you feel about

if (...) {
    int err = some_function(...);
    
    if (err) {
        return err;
    }
} else {
    int err = some_other_function(...);
    
    if (err) {
        return err;
    }
}

compared to

int err;

if (...) {
    err = some_function(...);
    
    if (err) {
        return err;
    }
} else {
    err = some_other_function(...);
    
    if (err) {
        return err;
    }
}

[Thalley]

Empty lines after lines containing only }:

OK:

if (val) {
    /* FOO */
}

return val;

Not OK:

if (val) {
    /* FOO */
}
return val;

TBD:

switch (val_1) {
case 1:
	break;
case 2:
	switch (val_2) {
	case 1:
		break;
	default:
		break;
	}
	break;
default:
	break;
}

[Thalley]

Alphabetically ordered declarations:

float my_float;
int my_int;
int my_int_with_initialization = 5;
struct my_struct_t my_struct; 

[jhedberg]

I've never seen such a rule or code that looks like it follows it (I might have missed the usage though since I didn't know such a rule exists :)

[Thalley]

I've never seen it enforced, but it often comes up in discussions for this area, and was even in the link that Asbjørn posted on Discord: https://hisham.hm/2018/06/16/when-listing-repeated-things-make-pyramids/

[Thalley]

Explicit void cast of unused return values

OK

(void)memset(val, 0, size);
p = memset(val, 0, size);

NOT OK

memset(val, 0, size);

[jhedberg]

I don't think this one is Bluetooth specific, but applicable throughout the tree

[Thalley]

We don't have any rules for it, right?

Perhaps memset is a bad example, as that is often void casted (for some reason), but there's not really any reason (beyond consistency) that memset should be treated different from e.g. net_buf_add_mem, so it could also have been

OK

(void)net_buf_add_mem(buf, mem, size);
p = net_buf_add_mem(buf, mem, size);

NOT OK

net_buf_add_mem(buf, mem, size);

[Thalley]

Unsigned values shall be suffixed with U:

OK:

uint8_t my_val = 6U;
uint64_t my_long_val = 123ULL;
int8_t my_int = 5;

if (my_val == 6U) { ... }

Not OK:

uint8_t my_val = 6;
uint64_t my_long_val = 123;

if (my_val == 6) { ... }

[Thalley]

No space between value retrievel and check

OK:

err = some_func();
if (err) {
    return err;
}

Not OK:

err = some_func();

if (err) {
    return err;
}

[Thalley]

Naming scheme of public/internal/static functions

For the sake of argument, I'll use the bt_conn namespace as an example.

Public function yyyy:

int bt_conn_yyyy();

Internal function yyyy (e.g. in conn_internal.h):

int bt_conn_yyyy()

Static function yyyy (e.g. in conn.c):

static int conn_yyyy();
// or
static int yyyy();

[Thalley]

enums instead of #define blocks in public API

Unless absolutely necessary that we use specific size constant values, enum is preferred over #define blocks.

Preferred

enum {
    BT_COMMON_VALUE_1,
    BT_COMMON_VALUE_2,
    BT_COMMON_VALUE_3,
    BT_COMMON_VALUE_4,
} bt_common_value;

Not preferred:

#define BT_COMMON_VALUE_1 1
#define BT_COMMON_VALUE_2 2
#define BT_COMMON_VALUE_3 3
#define BT_COMMON_VALUE_4 4

The reasoning here is that functions can take bt_common_value value as parameter which makes it very easy for the user to determine the value range, even without proper documentation.

[Thalley]

Size of array after array in parameters

The size/length parameter in an array should come after the array in the parameter list.

OK:

int foo(uint8_t *data, size_t data_len);

Not OK

int foo(size_t data_len, uint8_t *data);

[Thalley]

Locking due to inactivity