Propose Documentation for Software Best Practices, Coding Conventions, and Style #358

gerickson · 2020-04-15T00:22:58Z

Problem

CHIP will be composed of much software contributed from participating member organizations. However, for any new CHIP code, there should be documentation for software best practices, coding conventions, and style.

Summary of Changes

Proposed a documentation guide, in AsciiDoc format, for software best practices, coding conventions, and style.

Fixes #335

gerickson · 2020-04-15T16:13:42Z

Reviewers may find it easier to review the attached PDF rendering of this: CODING_STYLE_GUIDE.pdf

woody-apple

Can we use ascii doc here, given we're using that for other things.

gerickson · 2020-04-15T23:26:24Z

Can we use ascii doc here, given we're using that for other things.

@rwalker-apple and I will try that, just need to get something that will preserve the desired formatting and automatically get us from the gDoc form to AsciiDoc automatically on the first pass without hand-writing it.

gerickson · 2020-04-23T07:50:59Z

Can we use ascii doc here, given we're using that for other things.

Done; now in AsciiDoc.

gerickson · 2020-04-23T07:51:29Z

Can we use ascii doc here, given we're using that for other things.

@rwalker-apple and I will try that, just need to get something that will preserve the desired formatting and automatically get us from the gDoc form to AsciiDoc automatically on the first pass without hand-writing it.

Done; now in AsciiDoc.

rwalker-apple · 2020-04-24T01:25:49Z

Nice work!

I think the best way to iterate on this is to accept as-is and use PRs to debate updates. There are some bits that don't match current tree rules, but fallback to "When in Rome..."

@woody-apple any thoughts?

gerickson · 2020-04-24T20:08:48Z

@tpmanley or @BroderickCarlin?

gerickson · 2020-04-24T20:09:04Z

Can we use ascii doc here, given we're using that for other things.

Resolved.

woody-apple · 2020-04-25T04:28:37Z

I’ll work on comments this weekend and early next week, but I’d like to pare down some of this to add flexibility for developers.

bukepo · 2020-04-29T08:31:36Z

Can we use ascii doc here, given we're using that for other things.

@woody-apple it seems asciidoc/asciidoc is not maintained anymore. Should we use it or choose another tool?

rwalker-apple · 2020-04-29T19:00:34Z

Can we use ascii doc here, given we're using that for other things.

@woody-apple it seems asciidoc/asciidoc is not maintained anymore. Should we use it or choose another tool?

asciidoc maintenance and support has transitioned to asciidoctor, and is still supported by pandoc and Google Docs

rwalker-apple · 2020-05-04T18:41:00Z

@woody-apple have you had time to review?

woody-apple

I'm struggling a little bit with the scope of the document here. I like the Doxygen push, as well as much of the C/C++ pushes, but this feels a little overly prescriptive - as well as duplicates clang-format/restyled.

Can this be reduced somehow to something more palatable for people to consume + read? (This took me a while to review...)

woody-apple · 2020-05-09T00:12:07Z

docs/style/coding/CODING_STYLE_GUIDE.adoc

+
+image:CODING_STYLE_GUIDE-figure1.png[Figure 1. Graphical summary of the
+qualitative and quantitative applicability to Project CHIP software.]
+


This is very tied to C/C++. We will have lots of Objective C and Swift Code, as well as I'm assuming Java, etc - I don't want to give the impression in a document that we discourage other languages.

Yes, the original source of this document was applied to shared, horizontal embedded C/C++ infrastructure projects so this document targeted that world.

We can definitely create a taxonomy of documents/guides fo other languages / platforms. I'm open to suggestions and crowd-sourcing. This was a bootstrap effort to get us started that @rwalker-apple and I suggested.

I think I'd really like to pare down the scope of this document, to something that is 2-3 pages max, so people can really digest this. I think the verbosity here makes it something that nobody will actually read... (maybe that's just me)

woody-apple · 2020-05-09T00:13:03Z

docs/style/coding/CODING_STYLE_GUIDE.adoc

+leveraged through toolchain-compatibility preprocessor macros.
+
+|link:#h.cdtfddg825xl[2.2] a|
+Project CHIP embedded software development uses the ISO14882:2011 (aka


Can we define embedded vs non embedded systems?

Additionally, I assume we're supporting assembly where required?

Definitely can support assembly where required. My experience has been that it's vanishingly rare and those that need to deal with it, know what they're doing enough that we didn't need to be prescriptive about it.

I feel like we could distill this down to something might lighter and more digestible. "By default, we should use C/C++, but are open to other languages on a case by case basis."

woody-apple · 2020-05-09T00:14:04Z

docs/style/coding/CODING_STYLE_GUIDE.adoc

+specifically, the use of the non-local scope inline functions should be
+avoided.
+
+|link:#h.w81efp46u07r[3.1.2.2] |Inline functions should be used


Why? Not sure I agree/understand. Aside from that we should avoid "thou shalt" - unless it's explained.

The up front content is a summary table. The summary table links to the full content along with motivation and rationale. Let me know what you think after the motivation and rationale.

I think we should handle these case by case, just feels odd to have this be stated unilaterally.

woody-apple · 2020-05-09T00:14:28Z

docs/style/coding/CODING_STYLE_GUIDE.adoc

+|link:#h.tjgjnfdzmvlt[3.1.2.4] |There should be no calls to the
+functions `setjmp` or `longjmp`.
+
+|link:#h.96ozmz48upvf[3.1.2.5] |There should be no calls to the C/C{plusplus}


Disagree here. These can be highly valuable. Why are we denying these?

The up front content is a summary table. The summary table links to the full content along with motivation and rationale. Let me know what you think after the motivation and rationale.

Totally agreed on the usefulness. We've encoded that usefulness mostly in the context of nlassert.h or CodeUtils.h macros that better decorate the usage.

woody-apple · 2020-05-09T00:14:55Z

docs/style/coding/CODING_STYLE_GUIDE.adoc

+|link:#h.w81efp46u07r[3.1.2.2] |Inline functions should be used
+judiciously.
+
+|link:#h.lznwwfek3mml[3.1.2.3] |There should be one `return` statement per


Disagree here, I understand some imported code bases like this, others won't - as well as more modern development. Is there a rationale here?

The up front content is a summary table. The summary table links to the full content along with motivation and rationale. Let me know what you think after the motivation and rationale.

woody-apple · 2020-05-09T00:43:37Z

docs/style/coding/CODING_STYLE_GUIDE.adoc

+such files by using a dash (“-”) in the file name to indicate the
+subcomponent or subset of functionality.
+
+File names should be all lower case.


This doesn't make sense in Darwin land. Can we do this on a review basis?

Agreed; we need to identify a taxonomy of style documents for the project.

Maybe we can just remove this item then?

woody-apple · 2020-05-09T00:44:04Z

docs/style/coding/CODING_STYLE_GUIDE.adoc

+[[h.dux1t2sfd7li]]
+=== 4.2.        Naming
+
+Names should be descriptive but not overly so and they should give some


ObjectiveC and Swift can be very verbose. Can we apply this at review?

We need to identify a taxonomy of style documents for the project.

woody-apple · 2020-05-09T00:44:54Z

docs/style/coding/CODING_STYLE_GUIDE.adoc

+[[h.7g2jekkhysl3]]
+====== 4.2.2.2.1.        Motivation and Rationale
+
+This is previously-agreed-upon long-standing convention.


Not sure where this was agreed on? I don't disagree, but seems like there could good exceptions to this.

Bad motivation and rationale here; this is a vestige of the document copy-and-paste. We can delete or come up with a legitimate motivation and rationale.

Let's delete please :)

woody-apple · 2020-05-09T00:45:21Z

docs/style/coding/CODING_STYLE_GUIDE.adoc

+[[h.145w2jejpii6]]
+====== 4.2.2.3.1.        Motivation and Rationale
+
+This is previously-agreed-upon long-standing convention.


Bad motivation and rationale here; this is a vestige of the document copy-and-paste. We can delete or come up with a legitimate motivation and rationale.

Let's delete please :)

woody-apple · 2020-05-09T00:46:29Z

docs/style/coding/CODING_STYLE_GUIDE.adoc

+[[h.h1hnyi16kha7]]
+==== 4.5.2.        Indentation
+
+Indentation shall be 4 space characters.


Restyled handles all this automatically... do we need to codify these in multiple places?

See prior comment about "What the...?!". If we don't need it, we can drop this/these.

I think we should drop most/all style stuff, and collapse it to a single item:
• Style must pass clang-format, .editorconfig, .pretty, etc.

gerickson · 2020-05-12T05:23:54Z

I'm struggling a little bit with the scope of the document here. I like the Doxygen push, as well as much of the C/C++ pushes, but this feels a little overly prescriptive - as well as duplicates clang-format/restyled.

Can this be reduced somehow to something more palatable for people to consume + read? (This took me a while to review...)

Generally, this was copy-and-pasted from an internal document. We can definitely flag things we don't like and eliminate or re-work them.

In terms of clang-format/restyled, the automation is great and there may be no need to document the "Why did you chose this option for clang-format?". Those things that are documented were those we at least found internally tended to generate the most "What the...!?" conversations where the motivation and rationale were useful.

BroderickCarlin · 2020-05-12T05:43:33Z

docs/style/coding/CODING_STYLE_GUIDE.adoc

+
+|link:#h.imbb237rmsjp[3.1.2.12] a|
+Read-only methods, global variables, stack variables, or data members
+are read-only should be qualified using the C or C{plusplus} `const` qualifier.


nit: that are

BroderickCarlin · 2020-05-12T05:53:03Z

docs/style/coding/CODING_STYLE_GUIDE.adoc

+
+|link:#h.vuy3kslcw1c0[3.2.7] |Code shall not use exceptions.
+
+|link:#h.s7u364eurjpt[3.2.7.2] |In the rare case that Proect CHIP code


nit: Project

BroderickCarlin · 2020-05-12T06:07:23Z

docs/style/coding/CODING_STYLE_GUIDE.adoc

+years. Project CHIP and the source code it has produced have only
+existed for the last five of those 20 years.


Guessing this is left over from a previous project, and even though this justification is even more relevant given the infancy of CHIP, we should fix the dating

woody-apple · 2020-05-12T22:07:46Z

@gerickson Thanks for all the time on this document! Can we find a way to pare this down to a 1-3 page document that people can read in maybe 10 minutes? I think that's the only way to have this be actually impactful - we need it to be consumable by our new developers :)

gerickson · 2020-05-12T22:10:56Z

@gerickson Thanks for all the time on this document! Can we find a way to pare this down to a 1-3 page document that people can read in maybe 10 minutes? I think that's the only way to have this be actually impactful - we need it to be consumable by our new developers :)

That was the thinking on the up front summary, those what want the TL;DR can stop there. Those that want the motivation and rationale can read further.

andy31415 · 2020-05-20T14:55:29Z

docs/style/coding/CODING_STYLE_GUIDE.adoc

+File names should be prefixed with ‘chip’.
+
+File names should follow the pattern
+_chip<component>[-<subcomponent>].\{c,cpp,h,hpp,m,mm}_.


Should we standardize if h or hpp should be used for C++ files? I have seen that we predominantely use .h, however there are a few .hpp files and I found that inconsistent.

It would be nice if we could either say "everything is h" or "C/C-compatible is h, CPP is hpp". I would favour "everything is h" myself.

andy31415 · 2020-05-20T14:58:03Z

docs/style/coding/CODING_STYLE_GUIDE.adoc

+File names should be prefixed with ‘chip’.
+
+File names should follow the pattern
+_chip<component>[-<subcomponent>].\{c,cpp,h,hpp,m,mm}_.


I believe this should be guided based on how we include things.

If we generate a separate include directory, I would like to #include <chip/connection.h> instead of #include <chip/chip-connection.h> as the latter seems a bit smurf-named to me. As prior art for the shorter include, openssl includes openssl/aes.h rather than openssl/OpenSslAes.h. I believe shorter includes are more prevalent within the industry and developers are more familiar with it.

andy31415 · 2020-05-20T15:17:30Z

docs/style/coding/CODING_STYLE_GUIDE.adoc

+[[h.ft3tkwpjkeex]]
+==== 3.2.4.        C{plusplus} Standard Library
+
+The use of the C{plusplus} Standard Library shall be avoided.


Identifying subsets may be useful instead of a blanket removal. While anything that uses dynamic memory allocation is bad, there are things that are probably useful, like algorithms or things that do not use memory allocation.

I would propose a whitelist approach here instead. I am thinking of <array>, <algorithm>, <climits>, <cmath> and such which likely do not introduce overhead but are very useful to have. Even std::unique_ptr may be useful for RAII (although more annoying to use due to custom deallocators when using the pool allocators).

With the note that unique_ptr probably requires -fno-exceptions to not generate stack unwinding. I am not sure if we are ready to say chip compiles with that flag though even though we require no exception usage.

woody-apple · 2020-06-29T20:14:47Z

@rwalker-apple @gerickson per our last chat - what are the next steps here?

gerickson · 2020-06-29T20:59:29Z

@rwalker-apple @gerickson per our last chat - what are the next steps here?

I don't think there is one / are any. At minimum, the Doxygen content got factored out into a standalone document.

The last discussion ended with a mandate that we spend no more cycles on this. If there are things that everyone comes to agreement across companies (and, at that, there does not even seem to be alignment within companies), we can bubble those things up and memorialize them in a from-empty-to-plus-plus approach rather than in a as-is-to-minus-minus approach.

woody-apple · 2020-06-29T21:52:56Z

The last discussion ended with a mandate that we spend no more cycles on this. If there are things that everyone comes to agreement across companies (and, at that, there does not even seem to be alignment within companies), we can bubble those things up and memorialize them in a from-empty-to-plus-plus approach rather than in a as-is-to-minus-minus approach.

@rwalker-apple @gerickson I suggest we close this for now, then open up an organic document as things come in? Thoughts?

CLAassistant · 2020-07-01T18:24:59Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

rwalker-apple · 2020-07-01T19:10:31Z

The last discussion ended with a mandate that we spend no more cycles on this. If there are things that everyone comes to agreement across companies (and, at that, there does not even seem to be alignment within companies), we can bubble those things up and memorialize them in a from-empty-to-plus-plus approach rather than in a as-is-to-minus-minus approach.

@rwalker-apple @gerickson I suggest we close this for now, then open up an organic document as things come in? Thoughts?

Fine with me, but can we get that document up first?

gerickson · 2020-07-08T18:14:11Z

Closing in lieu of #1504.

gerickson · 2020-07-08T18:28:24Z

The last discussion ended with a mandate that we spend no more cycles on this. If there are things that everyone comes to agreement across companies (and, at that, there does not even seem to be alignment within companies), we can bubble those things up and memorialize them in a from-empty-to-plus-plus approach rather than in a as-is-to-minus-minus approach.

@rwalker-apple @gerickson I suggest we close this for now, then open up an organic document as things come in? Thoughts?

Fine with me, but can we get that document up first?

See here for such a document: #1504

gerickson added documentation Improvements or additions to documentation enhancement New feature or request labels Apr 15, 2020

gerickson requested review from anush-apple, BroderickCarlin, chrisdecenzo, hawk248, jelderton, robszewczyk, tpmanley and woody-apple as code owners April 15, 2020 00:22

jelderton approved these changes Apr 15, 2020

View reviewed changes

woody-apple requested changes Apr 15, 2020

View reviewed changes

gerickson force-pushed the user/gerickson/github-issue-335 branch from 441db33 to 62e0442 Compare April 23, 2020 07:48

BroderickCarlin approved these changes Apr 25, 2020

View reviewed changes

saurabhst approved these changes Apr 25, 2020

View reviewed changes

woody-apple requested changes May 9, 2020

View reviewed changes

BroderickCarlin reviewed May 12, 2020

View reviewed changes

Initial revision.

f96542f

gerickson force-pushed the user/gerickson/github-issue-335 branch from 7573d23 to f96542f Compare May 12, 2020 19:39

andy31415 reviewed May 20, 2020

View reviewed changes

woody-apple marked this pull request as draft June 30, 2020 04:35

gerickson mentioned this pull request Jul 8, 2020

Propose Documentation for Software Best Practices, Coding Conventions, and Style #1504

Merged

gerickson closed this Jul 8, 2020


		image:CODING_STYLE_GUIDE-figure1.png[Figure 1. Graphical summary of the
		qualitative and quantitative applicability to Project CHIP software.]


		\|link:#h.vuy3kslcw1c0[3.2.7] \|Code shall not use exceptions.

		\|link:#h.s7u364eurjpt[3.2.7.2] \|In the rare case that Proect CHIP code

		years. Project CHIP and the source code it has produced have only
		existed for the last five of those 20 years.

Propose Documentation for Software Best Practices, Coding Conventions, and Style #358

Propose Documentation for Software Best Practices, Coding Conventions, and Style #358

Conversation

gerickson commented Apr 15, 2020 • edited Loading

Problem

Summary of Changes

gerickson commented Apr 15, 2020

woody-apple left a comment

Choose a reason for hiding this comment

gerickson commented Apr 15, 2020

gerickson commented Apr 23, 2020

gerickson commented Apr 23, 2020

rwalker-apple commented Apr 24, 2020

gerickson commented Apr 24, 2020

gerickson commented Apr 24, 2020

woody-apple commented Apr 25, 2020

bukepo commented Apr 29, 2020

rwalker-apple commented Apr 29, 2020

rwalker-apple commented May 4, 2020

woody-apple left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerickson commented May 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BroderickCarlin May 12, 2020 • edited Loading

Choose a reason for hiding this comment

woody-apple commented May 12, 2020

gerickson commented May 12, 2020

Choose a reason for hiding this comment

andy31415 May 20, 2020 • edited Loading

Choose a reason for hiding this comment

andy31415 May 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woody-apple commented Jun 29, 2020

gerickson commented Jun 29, 2020

woody-apple commented Jun 29, 2020

CLAassistant commented Jul 1, 2020

rwalker-apple commented Jul 1, 2020

gerickson commented Jul 8, 2020

gerickson commented Jul 8, 2020

gerickson commented Apr 15, 2020 •

edited

Loading

BroderickCarlin May 12, 2020 •

edited

Loading

andy31415 May 20, 2020 •

edited

Loading

andy31415 May 20, 2020 •

edited

Loading