Re: [PATCH] main: Use static global rather than 'General' string literal.

On Dec 22, 2023, at 10:18 AM, Denis Kenzior <[email protected]> wrote:
>>  +/*
>> + * This is declared as 'const char *const' to effect an immutable
>> + * pointer to an immutable null-terminated character string such that
>> + * it ends up in .text, not .data (which would otherwise be the case
>> + * for a 'const char *' declaration), and with the 'static'
>> + * storage/scope qualifier, the compiler can optimize its use within
>> + * this file as it sees fit.
>> + */
> 
> I think you already had a similar comment in src/gateway.c.

Confirmed; I had added that per Marcel’s feedback.

>  Since this is something that makes sense to make into the accepted (and encouraged) pattern, would it make sense to document this in doc/coding-style.txt and avoid replicating this verbiage across the project?

I think so. I’d probably phrase something to the effect of:

    If there are no preprocessor requirements (such as concatenation), string constants should be declared at the narrowest
    possible scope as declared as 'const char *const' to effect an immutable pointer to an immutable null-terminated character
    string such that it ends up in .text, not .data (which would otherwise be the case for a 'const char *’ declaration).

    Unless the scope is cross-module, they should also be declared with the ‘static' storage/scope qualifier such that the
    compiler can locally optimize its use within the scope as it sees fit.

    If there are preprocessor requirements (such as concatenation), string constants should be declared as preprocessor
    definitions.

>> +static const char *const general_group_name = "General";
> 
> Given doc/coding-style.txt, item M3, should this be 'static const char * const'?

Unfortunately, checkpatch.pl would be in conflict with M3 and insists they be declared as I did.

I run checkpatch.pl as:

    checkpatch.pl --max-line-length=80 --no-signoff --no-tree --ignore SPLIT_STRING,INITIALISED_STATIC

since that seems to embody most of the style deviations I see in connman relative to what checkpatch.pl expects by default for the kernel. Ideally, there’d be a local wrapper in the project for checkpatch that specifies the desired options that reflect the project’s preferred style. As I’ve mentioned previously, in the future, I’d like to see a script in the project that uses clang-format to both verify and enforce project syntactic style.

> Also, since you're using this somewhat like a #defined constant, should this be capitalized for clarity?

Personally, I like to reserve all capitalized symbol names for the preprocessor as a convention.

Best,

Grant

-- 
Principal
Nuovations

[email protected]
http://www.nuovations.com/