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

Hi Grant,

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

Yeah, that sounds reasonable to me.  I think this change should be added to 
connman/ell/iwd/ofono

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

Hmm, that seems counter to what the linux kernel seems to prefer by a 8x margin:

[denkenz@archdev linux]$ grep -R 'const char \*const' * | wc -l
1595
[denkenz@archdev linux]$ grep -R 'const char \* const' * | wc -l
13650

> I run checkpatch.pl as:
> 
>      checkpatch.pl --max-line-length=80 --no-signoff --no-tree --ignore SPLIT_STRING,INITIALISED_STATIC

Which version of checkpatch are you running?  I see no warning when I run latest 
linux git version against such a diff:

diff --git a/src/main.c b/src/main.c
index 3ce8340fc90b..c41668a6de35 100644
--- a/src/main.c
+++ b/src/main.c
@@ -54,6 +54,8 @@

  #include "src/backtrace.h"

+static const char * const foobar = "sdf2434";
+

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

Yes, agreed.  I was hoping to play with CI over the holidays.

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

Okay, no strong feeling here.  Linux coding style doesn't really say much except:
"Names of macros defining constants and labels in enums are capitalized."

Having constants capitalized is a pretty general pattern though, and in ConnMan 
as well.  For example, gdchp/common.h:

static const uint8_t MAC_BCAST_ADDR[ETH_ALEN] __attribute__((aligned(2))) = {

static const uint8_t MAC_ANY_ADDR[ETH_ALEN] __attribute__((aligned(2))) = {

iwd did something pretty similar to what you're doing in this patch and 
approached it this way:
https://git.kernel.org/pub/scm/network/wireless/iwd.git/tree/src/knownnetworks.c#n69

Regards,
-Denis