Re: First draft of Coding standards in develop branch

[Andrei Emeltchenko]

Hi,

On Mon, Apr 25, 2016 at 09:03:17PM -0700, Sterling Hughes wrote:
> 
> >2) Should all local variables be defined at the beginning of the function? 
> >(my vote: yes)

should it be in the beginning of scope?

like:

myfunc
{
...
{
int i = 0;
}
}

Best regards 
Andrei Emeltchenko 

Re: First draft of Coding standards in develop branch

[will sanfilippo]

> On Apr 26, 2016, at 8:28 AM, Christopher Collins  wrote:
> 
> On Sun, Apr 24, 2016 at 10:08:09AM -0700, Sterling Hughes wrote:
>> Hi,
>> 
>> As we start to bring on new contributors, and operate as a project, its 
>> increasingly important that we document and agree upon coding standards. 
>>  I think we've done a good job of maintaining this consistency 
>> informally, but, now we need to vote and agree on project standards.
>> 
>> I've taken a first stab at this and committed it to the develop branch, 
>> folks can see it here:
>> 
>> https://github.com/apache/incubator-mynewt-core/blob/develop/CODING_STANDARDS.md
> 
> Thanks for putting this together Sterling.  I think it looks great.  My
> opinion is that a coding standards document should not be overly
> prescriptive.  Everyone has his own set of coding pet peeves; I suggest
> we try to keep those out of this document and keep it as short as
> possible.  Otherwise, people won't adhere to the document, or they will
> just hate writing code and they won't contribute as much.
> 
> For me, the important rules are:
>* Maximum line length
>* Brace placement
>* Typedefs
>* All-caps macros
>* Compiler extensions (e.g., packed structs).
> 
> The first three are already captured; I think the others should be
> addressed.  I think macros should always be in all-caps for reasons that
> everyone is probably familiar with. Unfortunately, I don't have a good
> rule for when extensions are acceptable.
> 
> I would also like to see a note about when it is OK to stray from the
> conventions.  There will be times (rarely) when adhering to the
> standards document just doesn't make sense.  "Zero-tolerance" rules
> always seem to pave the road to hell :).
+1. Even though I have preferences they are simply preferences. Specifying as 
little as possible that must be seems like a good idea.
> 
> Finally, there is one point in particular that I wanted to address:
> include guards in header files.  From the document:
> 
>* ```#ifdef``` aliasing, shall be in the following format, where
>the package name is "os" and the file name is "callout.h":
> 
>```no-highlight
>#ifndef _OS_CALLOUT_H
>#define _OS_CALLOUT_H
> 
> I am not sure I like this rule for the following two reasons:
>* A lot of the code base doesn't follow this particular naming
>  convention.
>* Identifiers starting with underscore and capital letter are
>  reserved to the implementation, so technically this opens the door
>  to undefined behavior.  
> 
> A leading capital E is also reserved by POSIX (e.g., EINVAL).  The
> naming convention I use is:
> 
>H_CALLOUT_
> 
> I would not consider this something to worry about, and I don't think we
> need to include a specific naming convention in the document.  However,
> insofar as we prescribe a naming convention, it should be one which
> avoids undefined behavior.
> 
> Thanks,
> Chris

Re: First draft of Coding standards in develop branch

[Christopher Collins]

I am not sure I would put it that way.  The POSIX headers are part of
the "implementation," so they are permitted to use the reserved
namespace.  Since mynewt code needs to build in sim (POSIX) and also
freestanding implementations, it is probably best to respect both
standards in our code.

Chris

On Tue, Apr 26, 2016 at 09:31:55AM -0700, Sterling Hughes wrote:
> 
> 
> On 4/26/16 9:22 AM, Christopher Collins wrote:
> > On Tue, Apr 26, 2016 at 09:06:37AM -0700, Sterling Hughes wrote:
> >> I think we do need a naming convention here - I'm fine adopting H_*.
> >> I'll point out that almost every POSIX or LIBC header I've ever seen
> >> uses underscore, and I believe this is only reserved in C++ -- that
> >> said, we need to be friendly to C++, and as you point out, we should
> >> follow a defined behavior.
> >
> > These identifiers are reserved in C as well.  From 7.1.3 of n1570:
> >
> >  All identifiers that begin with an underscore and either an
> >  uppercase letter or another underscore are always reserved for for
> >  any use.
> >
> > (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf)
> >
> 
> Hrm, you're right.  There are a lot of POSIX headers that have undefined 
> behavior. :-)
> 
> Sterling

Re: First draft of Coding standards in develop branch

[Sterling Hughes]

On 4/26/16 9:22 AM, Christopher Collins wrote:

On Tue, Apr 26, 2016 at 09:06:37AM -0700, Sterling Hughes wrote:

I think we do need a naming convention here - I'm fine adopting H_*.
I'll point out that almost every POSIX or LIBC header I've ever seen
uses underscore, and I believe this is only reserved in C++ -- that
said, we need to be friendly to C++, and as you point out, we should
follow a defined behavior.


These identifiers are reserved in C as well.  From 7.1.3 of n1570:

 All identifiers that begin with an underscore and either an
 uppercase letter or another underscore are always reserved for for
 any use.

(http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf)



Hrm, you're right.  There are a lot of POSIX headers that have undefined 
behavior. :-)


Sterling

Re: First draft of Coding standards in develop branch

[Christopher Collins]

On Tue, Apr 26, 2016 at 09:06:37AM -0700, Sterling Hughes wrote:
> I think we do need a naming convention here - I'm fine adopting H_*. 
> I'll point out that almost every POSIX or LIBC header I've ever seen 
> uses underscore, and I believe this is only reserved in C++ -- that 
> said, we need to be friendly to C++, and as you point out, we should 
> follow a defined behavior.

These identifiers are reserved in C as well.  From 7.1.3 of n1570:

All identifiers that begin with an underscore and either an
uppercase letter or another underscore are always reserved for for
any use.

(http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf)

Chris

Re: First draft of Coding standards in develop branch

[Sterling Hughes]

On 4/26/16 8:28 AM, Christopher Collins wrote:

On Sun, Apr 24, 2016 at 10:08:09AM -0700, Sterling Hughes wrote:

Hi,

As we start to bring on new contributors, and operate as a project, its
increasingly important that we document and agree upon coding standards.
   I think we've done a good job of maintaining this consistency
informally, but, now we need to vote and agree on project standards.

I've taken a first stab at this and committed it to the develop branch,
folks can see it here:

https://github.com/apache/incubator-mynewt-core/blob/develop/CODING_STANDARDS.md


Thanks for putting this together Sterling.  I think it looks great.  My
opinion is that a coding standards document should not be overly
prescriptive.  Everyone has his own set of coding pet peeves; I suggest
we try to keep those out of this document and keep it as short as
possible.  Otherwise, people won't adhere to the document, or they will
just hate writing code and they won't contribute as much.

For me, the important rules are:
 * Maximum line length
 * Brace placement
 * Typedefs
 * All-caps macros
 * Compiler extensions (e.g., packed structs).

The first three are already captured; I think the others should be
addressed.  I think macros should always be in all-caps for reasons that
everyone is probably familiar with. Unfortunately, I don't have a good
rule for when extensions are acceptable.



+1

I have never found a scenario where macros should be lower case.  I have 
found some where I thought it would make sense (e.g. min()), but in 
retrospect, MIN() or an inline function would have been just fine.



I would also like to see a note about when it is OK to stray from the
conventions.  There will be times (rarely) when adhering to the
standards document just doesn't make sense.  "Zero-tolerance" rules
always seem to pave the road to hell :).

Finally, there is one point in particular that I wanted to address:
include guards in header files.  From the document:

 * ```#ifdef``` aliasing, shall be in the following format, where
 the package name is "os" and the file name is "callout.h":

 ```no-highlight
 #ifndef _OS_CALLOUT_H
 #define _OS_CALLOUT_H

I am not sure I like this rule for the following two reasons:
 * A lot of the code base doesn't follow this particular naming
   convention.


A lot of it does :-)


 * Identifiers starting with underscore and capital letter are
   reserved to the implementation, so technically this opens the door
   to undefined behavior.

A leading capital E is also reserved by POSIX (e.g., EINVAL).  The
naming convention I use is:

 H_CALLOUT_

I would not consider this something to worry about, and I don't think we
need to include a specific naming convention in the document.  However,
insofar as we prescribe a naming convention, it should be one which
avoids undefined behavior.



I think we do need a naming convention here - I'm fine adopting H_*. 
I'll point out that almost every POSIX or LIBC header I've ever seen 
uses underscore, and I believe this is only reserved in C++ -- that 
said, we need to be friendly to C++, and as you point out, we should 
follow a defined behavior.


Sterling

Re: First draft of Coding standards in develop branch

[Christopher Collins]

On Sun, Apr 24, 2016 at 10:08:09AM -0700, Sterling Hughes wrote:
> Hi,
> 
> As we start to bring on new contributors, and operate as a project, its 
> increasingly important that we document and agree upon coding standards. 
>   I think we've done a good job of maintaining this consistency 
> informally, but, now we need to vote and agree on project standards.
> 
> I've taken a first stab at this and committed it to the develop branch, 
> folks can see it here:
> 
> https://github.com/apache/incubator-mynewt-core/blob/develop/CODING_STANDARDS.md

Thanks for putting this together Sterling.  I think it looks great.  My
opinion is that a coding standards document should not be overly
prescriptive.  Everyone has his own set of coding pet peeves; I suggest
we try to keep those out of this document and keep it as short as
possible.  Otherwise, people won't adhere to the document, or they will
just hate writing code and they won't contribute as much.

For me, the important rules are:
* Maximum line length
* Brace placement
* Typedefs
* All-caps macros
* Compiler extensions (e.g., packed structs).

The first three are already captured; I think the others should be
addressed.  I think macros should always be in all-caps for reasons that
everyone is probably familiar with. Unfortunately, I don't have a good
rule for when extensions are acceptable.

I would also like to see a note about when it is OK to stray from the
conventions.  There will be times (rarely) when adhering to the
standards document just doesn't make sense.  "Zero-tolerance" rules
always seem to pave the road to hell :).

Finally, there is one point in particular that I wanted to address:
include guards in header files.  From the document:

* ```#ifdef``` aliasing, shall be in the following format, where
the package name is "os" and the file name is "callout.h":

```no-highlight
#ifndef _OS_CALLOUT_H
#define _OS_CALLOUT_H

I am not sure I like this rule for the following two reasons:
* A lot of the code base doesn't follow this particular naming
  convention.
* Identifiers starting with underscore and capital letter are
  reserved to the implementation, so technically this opens the door
  to undefined behavior.  

A leading capital E is also reserved by POSIX (e.g., EINVAL).  The
naming convention I use is:

H_CALLOUT_

I would not consider this something to worry about, and I don't think we
need to include a specific naming convention in the document.  However,
insofar as we prescribe a naming convention, it should be one which
avoids undefined behavior.

Thanks,
Chris

Re: First draft of Coding standards in develop branch

[Johan Hedberg]

Hi,

On Mon, Apr 25, 2016, will sanfilippo wrote:
> > 9) I think the 79 character line length is not really helpful.  I¹d rather
> > see slightly longer lines.  I often prefer To use longer names for example
> > int res = hal_adc_get_resolution_mvolts(padc) to make it clear what is
> > going on and the units, but that may make lots of wrapping with an 80
> > column limit.  This simple statement used 45 characters already.  I know
> > its been standard forever, but screens are 5x wider than they used to be.
> > Can¹t we stretch this to 120. I hope you are reading this email with
> > 80 columns!!
> Good luck getting others to change this :-) I would be fine personally.

I almost always have my editor split with at least two side-by-side
views of the source code so I can easily compare different places and
copy/move code around. At least on my laptop display I need to have a
large enough font that if the line width was much greater than 80 this
wouldn't be possible.

> >>> This:
> >>> #define PRETTY(0)
> >>> #define VERY_PRETTY   (1)
> >>> #define BEAUTIFUL (2)

What's with the obsession of putting "atomic" things like integers
inside an extra pair of parenthesis? :) Seems excessive to me. Should
the coding style document make a note of this? (which ever way is the
preferred convention)

Another thing, should this document also cover git commit message
conventions? Summary line prefixes, line widths, etc?

Johan

Re: First draft of Coding standards in develop branch

[will sanfilippo]

> On Apr 25, 2016, at 9:22 PM, p...@wrada.com wrote:
> 
> 
> My notes. Only a few strong opinions.
> 
> 0) Way to go Sterling.  Better sooner than later for this.
> 1) when I was writing the hal I wrote a hal/hal_adc.h (public API)  and
> mcu/hal_adc.h (private API name for the BSP to set MCU specific
> parameters). 
> I was burned because of the #ifndef __HAL_ADC_H__.   I replace them with
> __HAL_HAL_ADC_H__ and __MCU_HAL_ADC_H__.  Really, there was probably not
> a need to have the BSP include the public API, but I believe there was
> some 
> include path that surprised me.  If we are serious about using the
> directory 
> as a namespace, we had better include it in the header include protection.
> These types of errors can be hard to detect.
+ 1.
> 2) I¹m a fan of macros in upper case only.
> 3) Regarding typedefs, can they be used for static functions. Seems that
> this makes things readable and doesn¹t cause the harm you mention.
> 4) Should we be picky about the use of const?
Please god say no :-)
> 5) any rules or naming conventions for enums?
> 6) any guidelines on file names. Is there a name length limit.  Probably
> should make names all lower case. Should the file name and function name
> match for public functions? (e.g. hal_adc.h contains hal_adc_init() )
I thought this was mentioned?
> 7) Muti-line comments formatted like in our apache header.
> 8) Any convention on line break character \?
> 9) I think the 79 character line length is not really helpful.  I¹d rather
> see slightly longer lines.  I often prefer To use longer names for example
> int res = hal_adc_get_resolution_mvolts(padc) to make it clear what is
> going on and the units, but that may make lots of wrapping with an 80
> column limit.  This simple statement used 45 characters already.  I know
> its been standard forever, but screens are 5x wider than they used to be.
> Can¹t we stretch this to 120. I hope you are reading this email with
> 80 columns!!
Good luck getting others to change this :-) I would be fine personally.
> 10) any other comment info like author or date at the top of the file ?
> 11) It always bums me out to see opposite conventions on parenthesis for
> functions and other code blocks.  For example suppose I do this.
> 
> void
> Foo(int x) 
> {
>/* some code with a conditional code block */
>if (this or that) {
> /* some code in a separate block that is conditional */
>}
> 
>/* an unconditional code block with good reason */
>{
> uint8_t local_tmp_for_calc;
> /* do a computation with some local variable and make
>  * it clear it has limited scope */
>}
> 
>switch(condition) {
>case value:
>{
>  uint8_t a_temp_i_only_need_here;
>  /* do a computation with a local variable with limited
>   * scope */
>  Break;
>}
>}
> }
> 
> I get why we want to have that lingering parenthesis on the end of the
> if and switch to make the code more succinct, but it seems at odds with
> the other code blocks.  Maybe its my bad style, but I occasionally use
> code blocks in case statements and free-standing functions to do a local
> calculation with a variable that I want to make clear is only valid in a
> limited scope (as opposed to declaring it at the top of the function).
> This leaves my code looking inconsistent because the if and switch have
> one style code block and the case, free, and function have another.
It is just me personally, but the switch/case above is hard for me to read 
(because of the {} around the guts of the case).
So I would vote -1 for that.
> 
> 
> 
> 
> On 4/25/16, 8:48 PM, "Sterling Hughes"  wrote:
> 
>> 
>> 
>> On 4/25/16 8:43 PM, will sanfilippo wrote:
>>> Proposed Changes:
>>> 
>>> * A function prototype in a header file may (or should?) be a single
>>> line (i.e. type and name can be on same line).
>>> * Global variables should be prefaced by g_
>>> 
>>> Comments:
>>> * I dont see anything here regarding ³alignment² of various things.
>>> Should we be adding these to the coding style? For example:
>>> 
>>> This:
>>> #define PRETTY  (0)
>>> #define VERY_PRETTY (1)
>>> #define BEAUTIFUL   (2)
>> 
>> You used tabs here - so it shows unaligned in email :-), but I get the
>> point and agree.  I don't feel too strongly about '#define' alignment,
>> but am happy to add it, I do it anyway.
>> 
>>> 
>>> Not:
>>> #define UGLY (0)
>>> #define REALLY_UGLY (1)
>>> #define HIDEOUS (2)
>>> 
>>> ‹ or ‹
>>> 
>>> This:
>>> struct my_struct
>>> {
>>> int ms_foo1;
>>> uint16_t ms_foo2;
>>> struct qelem elem;
>>> }
>>> 
>>> Not:
>>> struct my_struct
>>> {
>>> int ms_foo1;
>>> uint16_tfoo2;
>>> struct qelemelem;
>>> }
>> 
>> +1 for this one.
>> 
>>> 
>>> Questions:
>>> * I presume that outside code not written to this style will not be
>>> modified? For example, 

Re: First draft of Coding standards in develop branch

[Sterling Hughes]

On 4/25/16 8:57 PM, will sanfilippo wrote:

Argh! I thought I had the stupid editor set to insert spaces for tabs. Dang! Oh 
well, at least you got the point :-)

* I would vote for macros all uppercase.
* I feel strongly about define alignment but not so much about alignment within 
structure definitions although I think I align things in structures generally.

Oh, some other good ones to discuss:
1) Should we allow initialization of local variables when they are defined. (my 
vote: no)


i agree, although some people may like to do this.  i don't like it.


2) Should all local variables be defined at the beginning of the function? (my 
vote: yes)




_definitely_.

sterling

Re: First draft of Coding standards in develop branch

[will sanfilippo]

Argh! I thought I had the stupid editor set to insert spaces for tabs. Dang! Oh 
well, at least you got the point :-)

* I would vote for macros all uppercase.
* I feel strongly about define alignment but not so much about alignment within 
structure definitions although I think I align things in structures generally.

Oh, some other good ones to discuss:
1) Should we allow initialization of local variables when they are defined. (my 
vote: no)
2) Should all local variables be defined at the beginning of the function? (my 
vote: yes)



> On Apr 25, 2016, at 8:48 PM, Sterling Hughes  wrote:
> 
> 
> 
> On 4/25/16 8:43 PM, will sanfilippo wrote:
>> Proposed Changes:
>> 
>> * A function prototype in a header file may (or should?) be a single line 
>> (i.e. type and name can be on same line).
>> * Global variables should be prefaced by g_
>> 
>> Comments:
>> * I dont see anything here regarding “alignment” of various things. Should 
>> we be adding these to the coding style? For example:
>> 
>> This:
>> #define PRETTY   (0)
>> #define VERY_PRETTY  (1)
>> #define BEAUTIFUL(2)
> 
> You used tabs here - so it shows unaligned in email :-), but I get the point 
> and agree.  I don't feel too strongly about '#define' alignment, but am happy 
> to add it, I do it anyway.
> 
>> 
>> Not:
>> #define UGLY (0)
>> #define REALLY_UGLY (1)
>> #define HIDEOUS (2)
>> 
>> — or —
>> 
>> This:
>> struct my_struct
>> {
>>  int ms_foo1;
>>  uint16_t ms_foo2;
>>  struct qelem elem;
>> }
>> 
>> Not:
>> struct my_struct
>> {
>>  int ms_foo1;
>>  uint16_tfoo2;
>>  struct qelemelem;
>> }
> 
> +1 for this one.
> 
>> 
>> Questions:
>> * I presume that outside code not written to this style will not be 
>> modified? For example, another open source project has code that we adopt.
> 
> We should add a note: follow the coding standards of the original source is 
> my perspective.
> 
>> * I presume that if not explicitly stated as “dont do” you can do it. For 
>> example, do all macros have to be uppercase? I can have either MY_MACRO(x) 
>> or my_macro(x)?
>> 
> 
> Within reason.  We can still make fun of particularly ugly code. :-)
> 
> On macros, what are people's sense?  I prefer to have _ALL_ my macros 
> uppercased, but I didn't put that in there.  I like to know what is a macro 
> (upper-case), vs what is a function.
> 
> Sterling

Re: First draft of Coding standards in develop branch

[Vipul Rahane]

Hello,

The document is really nice and keeps the code clean avoiding any syntactical 
inconsistencies. 

Some of the previous companies I worked at followed MISRA C coding standards 
which was sought after by the automotive industry mostly in the UK. Do we want 
to take a look at it and think of adhering to some of the rules mentioned by 
them if not all. The automotive industry has been using ARM controllers for 
ages and take the MISRA standards quite seriously.

I could see several use cases:
1. Making our code safety critical, portable most of which we might be doing 
anyways.
2. Adoption by the automotive industry
3. Producing Lint free code (Less issues on doing Static code analysis - On 
running PC Lint/Coverity)

I would not worry about any of this if it was a single product we were 
targeting. Since it is an OS and we want people to adopt it, some of them might 
be from an automotive background who trust MISRA C standards more than they 
trust us. 

This might be a separate discussion and sorry to bring this up if it’s not 
needed here.

Having said all that, from a developers perspective I think the document is 
really good and does a very good job at addressing inconsistencies in the code. 
We might want to add an example of a C++ style comment: Something that we don’t 
want our code filled up with “//“ :-)

Regards,
Vipul Rahane

> On Apr 24, 2016, at 10:08 AM, Sterling Hughes  wrote:
> 
> Hi,
> 
> As we start to bring on new contributors, and operate as a project, its 
> increasingly important that we document and agree upon coding standards.  I 
> think we've done a good job of maintaining this consistency informally, but, 
> now we need to vote and agree on project standards.
> 
> I've taken a first stab at this and committed it to the develop branch, folks 
> can see it here:
> 
> https://github.com/apache/incubator-mynewt-core/blob/develop/CODING_STANDARDS.md
> 
> I think next steps are:
> 
> - If you have formatting fixes, or clarifications that don't change the 
> contents, just go ahead and edit the document.
> 
> - If you disagree with something in the document, or think it should be 
> changed in some way, respond back to this thread.
> 
> - If you have additions or rules that you don't think were captured, respond 
> back to this thread, along with the proposed text.  If nobody objects, I'll 
> just fold them into whatever other revisions we decide to make.
> 
> After we give this a round of discussion, I'll capture all the feedback and 
> do a rev of the document.  If nobody has any issues with that rev, we can 
> vote to officially adopt these coding standards.  I personally think this 
> should be an up or down vote.
> 
> Well - OK, let the conversations begin :-)
> 
> Sterling

First draft of Coding standards in develop branch

[Sterling Hughes]

Hi,

As we start to bring on new contributors, and operate as a project, its 
increasingly important that we document and agree upon coding standards. 
 I think we've done a good job of maintaining this consistency 
informally, but, now we need to vote and agree on project standards.


I've taken a first stab at this and committed it to the develop branch, 
folks can see it here:


https://github.com/apache/incubator-mynewt-core/blob/develop/CODING_STANDARDS.md

I think next steps are:

- If you have formatting fixes, or clarifications that don't change the 
contents, just go ahead and edit the document.


- If you disagree with something in the document, or think it should be 
changed in some way, respond back to this thread.


- If you have additions or rules that you don't think were captured, 
respond back to this thread, along with the proposed text.  If nobody 
objects, I'll just fold them into whatever other revisions we decide to 
make.


After we give this a round of discussion, I'll capture all the feedback 
and do a rev of the document.  If nobody has any issues with that rev, 
we can vote to officially adopt these coding standards.  I personally 
think this should be an up or down vote.


Well - OK, let the conversations begin :-)

Sterling