C++ Feature Whitelist/Blacklist

[Nancy Yuen]

Thanks Brendan for putting this list together and getting the discussion
going.
It's an important discussion to have.  A coding style or coding guidelines
is crucial developing software collaboratively and effectively.  Especially
since we all want OpenBMC to have other contributors eventually.
I'm still catching up on thread.  I wanted to give my reply to Patrick's
msg even though Brendan's has already replied.

On Wed, Nov 2, 2016 at 10:18 AM, Patrick Williams <patrick at stwcx.xyz> wrote:
> Our legacy product is somewhere between 1 and 3 MLOC.  Titus talked
> about the internal Google repository being on the order of 10 MLOC.
> Code bases this big certainly have unique problems and the code rules
> were put in place to mitigate some of these problems.  The two key
> points of Titus' talk were:
>
>     1) Have a style guide; tailor it to your situation.
>     2) Use your guide to encourage "good" and discourage "bad".
>
> I strongly feel that if we simply leap to a style guide targeting a
> monolithic N-MLOC codebase, we are taking on solutions that are not
> relevant to our own problems.  My expectation is that OpenBMC is a
> collection of small, self-contained repositories on the order of 5 -
> 50kLOC each.  The dependencies between repositories (interfaces) are
> purposefully segmented via the DBus interfaces, which represent a very
> different problem from a monolitic codebase where a new dependency is
> simply an #include away.  With small repositories, the code itself can
> "fit in the head" of a maintainer or active contributor fairly easily[1].

The features Brendan discusses may appear in the Google style guide but
they are equally applicable to smaller code bases.  In fact, I've worked on
embedded systems with a much smaller code base, prior to working at Google,
and the style guide had most, if not all, these items.

As for fitting in the head of the contributor or maintainer, I don't think
it's reasonable.  One of the points made during our meeting was that
openBMC should be a project other people can come in and contribute to.
Future contributors may or may not be devoting all their time to openBMC
and may not have the bandwidth to "fit" one of the repos in their head.

Btw, Google's codebase is not monolithic.

> My typical philosophy regarding coding practices tends to be fairly
> pragmatic.  I don't tend to see practices as "black" and "white" but
> more along a scale of how much justification you need to have to use a
> practice (more on this in the specifics, maybe).  The aspects that
> concern me more, when I am doing code reviews, are in the general sense:
>
>     1) Conciseness:
>         - Did you use far more lines of code to solve the problem than
>           necessary?  Those are lines we have to maintain.
>     2) Modern, widely-accepted practices:
>         - This is already spelled out in the 'contributing' doc, having
>           priority over any specific style statements. [2]
>     3) Performance:
>         - Favoring in order { Code size, CPU utilization, memory size }
>           due to the unique concerns of the OpenBMC project and current
>           technologies used.
>         - Macro-level optimization that does not violate conciseness and
>           clarity:
>             - Are the algorithms used of the appropriate algorithmic
>               complexity?
>             - Are you avoiding practices that would contribute to large
>               generated code sizes?

We're not suggesting the style guide be black and white.  Every rule has
it's exceptions.  But rules provide structure which is necessary when
trying to collaborate.

Thank you for this list.  It's general, but still helpful for those of use
sending code to you for review.

> Quantifying "code smell" into a set of non-ambiguous rules is a hard
> problem and, as such, one that I have avoided up until now.  I would
> prefer we start with industry recognized experts opinion[3] and then
> document deviations specific to OpenBMC as they come up.

I haven't read all of [3] in detail but,Brendan's list isn't meant to
replace any industry guideline, just spell out guidelines for a few things
that we feel are particularly important to writing concise, debuggable code.

And while it's hard to write a set of guidelines, it's a worthwhile task to
complete.  From [3] "Please remember that one purpose of a guideline is to
help someone who is less experienced or coming from a different background
or language to get up to speed."  Ideally, as OpenBMC is adopted, there
will be more contributors, so this guideline will help them.  Right now,
it'll help us collaborate.

I also want to echo Brendan's caution in his reply with regard to citing
industry experts.  [3] is not a list of do and don't dos by industry
experts.  Their abstract clearly states "Please try to verify or disprove
rules! In particular, we'd really like to have some of our rules backed up
with measurements or better examples"


> >     * Variables of class type with static storage duration (including
> > constants),
> CppCoreGuidelines I.22 and R.6 already have this point covered
> sufficiently to me without further elaboration.
> >     * Implicit conversion constructors and operators
> CppCoreGuidelines has a "To-do: Unclassified proto-rules" section which
> says "Avoid implicit conversions".

For these items, I think the CppCoreGuidelines are more or less in line
with what Brendan has proposed.

> > On the other hand, we do not think restrictions should be placed on
using
> > virtual
> > functions.
>
> I was not proposing an elimination or prohibition on virtual functions,
> but a "justification required".  As in, you need to be prepared to
> answer how this is better and why compile-time polymorphism would be
> insufficient.
>
> CppCoreGuidelines C.10 suggests effectively the same thing: "You need a
> reason for using a hierarchy."

CppCoreGuidelines do list reasons for using a hierarchy in C.120-122.  I
think these are sufficient for most of the cases of inheritance that we'd
be interested in.


----------
Nancy
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20161104/c77d4074/attachment.html>