C++ Feature Whitelist/Blacklist

C++ Feature Whitelist/Blacklist

[Brendan Higgins]

[-- Attachment #1: Type: text/plain, Size: 1471 bytes --]

At the OpenBMC meetup last week we discussed creating an authoritative C++
style guide as well as a feature whitelist/blacklist. I am going to try to
get an early style guide out fairly soonish (this week or next) that we can
discuss, but while I do that I would like to bring up the topic of
explicitly supported/unsupported features to discuss on the mailing list.
First off, I think it is something that would have much more discussion and
stronger opinions that would be convenient on gerrit/I would like to start
the discussion earlier. I also think that there will probably only be a few
things on that list, so discussion via email should not be overly
burdensome.

At the meeting Patrick brought up two features to blacklist:

   - Streams and stream style operators.
   - Virtual functions/runtime polymorphism

In the case of virtual functions, I get the sense that the use is more
discouraged rather than completely forbidden.

For the whitelist, the only current candidate is:

   - exceptions

In general, I suppose the notion of a whitelist is rather silly, but
exceptions have historically been a controversial feature of C++, so I
figured it best to explicitly call out.

I do not want to mischaracterize the arguments in favor of this proposal;
it also does not seem fair to offer up a viewpoint with little explanation
and critique it in the same breath, so instead I will post a follow up
email with what I propose for the whitelist/blacklist.

Cheers

[-- Attachment #2: Type: text/html, Size: 1632 bytes --]

Re: C++ Feature Whitelist/Blacklist

[Brendan Higgins]

[-- Attachment #1: Type: text/plain, Size: 2238 bytes --]

Following up with what I think the whitelist/blacklist should be:

Blacklist:

    * Variables of class type with static storage duration (including
constants),
      plain old data types (POD) are allowed
        - Main reason for this is that it is hard to keep track of when
these
          variables get initialized
    * Implicit conversion constructors and operators
        - Makes code confusing and difficult to debug
    * Multiple Inheritance (unless at most one of the parent classes is an
      interface i.e. only has pure virtual methods)
        - Detracts from readability
    * Non-const-non-r-value references
        - Use of const references and r-value references are encouraged
        - For non-const-non-r-value-references use pointers
        - Use where required or needed to maintain consistency with the STL
is
          allowed
    * Exceptions
        - Functions have to explicitly opt out of exceptions with noexcept,
          cleaner to assume no function throws exceptions
        - No way to force exception handling (forcing handling of status
objects
          is possible with the warn_unused_result attributes supported by
gcc
          and clang).
        - Exceptions are not typed
        - Does not play nice with legacy code
        - We are interested in porting existing code to OpenBMC most of
which is
          not exception safe

Other considerations:
    * Doing Work in Constructors
        - Avoid virtual method calls in constructors, and avoid
initialization
          that can fail if you can't signal an error.
        - Don’t throw exceptions in constructors or destructors
        - Generally, anything that can fail should be kept out of
constructors
            - Maybe a separate Init function

Again, the above is by no means exhaustive; it only lists features which we
propose should be completely banned.

On the other hand, we do not think restrictions should be placed on using
virtual
functions.

Also, we do not feel strongly about stream operators, but would in general
object to using it as the primary grounds to not use a particular library,
as an
example.

(sorry about any html in the previous email)

[-- Attachment #2: Type: text/html, Size: 2830 bytes --]

Re: C++ Feature Whitelist/Blacklist

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 12641 bytes --]

At CppCon2014, Titus Winters gave a talk titled "Philosophy of Google's
C++ Style" where he gave background on why some of the "contentions"
rules in the GSG are in place.  I recognize and appreciate the concerns
that leads to these "style" rules.  On our large legacy product we had
similar rules in spirit, different in letter.

For OpenBMC, I believe we have to be aware that each group participating
may have different internal practices from the broader open source
community.  One approach could obviously be that we have a union of each
major contributor's internal practices, but that could lead to effectively
reducing the language to be nothing more than C-with-classes.

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

I do suspect that the majority of our "rules" will evolve around the
DBus interfaces, which are fairly unique to our project.

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?

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.

On Tue, Nov 01, 2016 at 05:53:44PM -0700, Brendan Higgins wrote:
> Following up with what I think the whitelist/blacklist should be:
> 
> Blacklist:
> 
>     * Variables of class type with static storage duration (including
> constants),
>       plain old data types (POD) are allowed
>         - Main reason for this is that it is hard to keep track of when
> these
>           variables get initialized

This proposal is a key example on why it is hard to quantify these types
of rules.  Everyone agrees with a general principle of "reducing
globals", but as spelled out this proposal would prohibit
initializer-list construction, std::string, and the pattern within our
IPMI plugins of "register on load".  These are all reasonable exceptions
that require little justification in my mind.

CppCoreGuidelines I.22 and R.6 already have this point covered
sufficiently to me without further elaboration.

>     * Implicit conversion constructors and operators
>         - Makes code confusing and difficult to debug

This would prohibit 'unique_ptr<T>::operator bool()' and
'string::string(const char*)', which are part of the STL and exist
for concise syntax.  I do agree with this in most cases but there are
cases where they both obvious and crisper.  A non-obvious conversion
should be prohibited.

CppCoreGuidelines has a "To-do: Unclassified proto-rules" section which
says "Avoid implicit conversions".

>     * Multiple Inheritance (unless at most one of the parent classes is an
>       interface i.e. only has pure virtual methods)
>         - Detracts from readability

Composition is fairly common practice in C++ which uses multiple
inheritance.  The sdbusplus bindings, as an example, have a specific
template to aid in the composition of N dbus interfaces into a single
C++ class. [4]

CppCoreGuidelines C.135 and C.136 seem to recommend multiple inheritance in
specific cases.

>     * Non-const-non-r-value references
>         - Use of const references and r-value references are encouraged
>         - For non-const-non-r-value-references use pointers
>         - Use where required or needed to maintain consistency with the STL
> is
>           allowed

I find this especially unnecessary, not recommended anywhere outside of
the GSG, and having a crippling effect to the language.  C++ is not a
procedural / OO programming language; it is also a functional
programming language.  The bulk of the STL is to facilitate a
functional programming style and many of the C++11 features (lambdas,
constexpr functions, etc.) take large inspiration from FP.  Eliminating
l-value reference makes FP techniques exceptionally harder and
syntactically clumsier.

A very common recommendation in "Exceptional Modern C++" is to define a
function like:
    template <typename T> foo(T&&)
This function operates on both l-value and r-value references
("universal references" as Meyers calls them).

CppCoreGuidelines F.15, F.16, F.17 all recommend using l-value
references by convention.

>     * Exceptions
>         - Functions have to explicitly opt out of exceptions with noexcept,
>           cleaner to assume no function throws exceptions

This was the "old way" in C++ and the standards body specifically
deprecated what you are suggesting.  I'm sure you could read their
rationale.

>         - No way to force exception handling (forcing handling of status
> objects
>           is possible with the warn_unused_result attributes supported by
> gcc
>           and clang).

In GCC, at least, function attributes can only be defined as part of
declaration and not definition.  That means you would have to forward
declare every function that returns your error type.

In every large project I have been involved with, an integer quickly
becomes insufficient to express "an error".  The projects all make their
own "error object".  According to Titus' talk, Google has one of these
internally as well.  So that means we're now talking about passing
around a heavy object by value or a pointer to one (which is always at
risk of leaking)?  What does this error look like?

CppCoreGuidelines state "Error handling using exceptions is the only
complete and systematic way of handling non-local errors in C++" and
have an entire chapter on how to do them "right".

Having a error return type likely eliminates any RVO and again plays a
huge diminishing role in using functional programming methods.  The two
approaches for output variables become:
    1) Fill-in out parameter reference.  [anti-FP]
    2) Return a tuple<error, real_return>.

Returning a tuple causes error handling to no longer be enforced with
warn_unused_result.

>         - Exceptions are not typed

Exceptions are strongly typed in the exact same type system as any
reference.  I don't know what this statement means.

>         - Does not play nice with legacy code
>         - We are interested in porting existing code to OpenBMC most of
> which is
>           not exception safe

Again this is the "lowest common denominator" argument, which I do not
accept.  We could make the same argument with our legacy codebase, which
I can assure you in this problem domain is much bigger than yours.  We
are making a concerted effort to not directly port any of our code we
are contributing to OpenBMC but to make sure it is modernized and
follows the practices with the rest of the codebase.

Any code you are planning to port would either need to go though the
same vetting process as a clean contribution (for acceptance into an
openbmc tree) or would be maintained in your own repositories.  If it is
something that is maintained in your own repositories, the only place
exceptions could be introduced is when you call base OpenBMC utilities,
like sdbusplus.  It would not be unreasonable to "catch" all exceptions
at the API level into base utilities and turn them into a return code.
(And in fact, I highly suspect you already have these abstracted out due
to your Google Mock test framework).

Since the majority of the repositories have self-contained programs that
expose DBus interfaces, what those programs do internally has no impact
on your ported programs that are interacting with those DBus interfaces.
DBus itself already has a DBus-Error type, which is distinct from method
return values, that you are free to turn into a return code instead of
an exception.

On exceptions in general, we have found that in our legacy code base
nearly 50% of the code paths were handling of error conditions.  Using
exceptions will greatly reduce and clean up this kind of code.  There
are arguments from the CppCoreGuidelines that code using exceptions can
actually be smaller and faster because of, for instance, better
optimization of the good path and less branch predictor failures.

> 
> Other considerations:
>     * Doing Work in Constructors
>         - Avoid virtual method calls in constructors, and avoid
> initialization
>           that can fail if you can't signal an error.
>         - Don’t throw exceptions in constructors or destructors
>         - Generally, anything that can fail should be kept out of
> constructors
>             - Maybe a separate Init function

CppCoreGuidelines C.44 and C.50 cover this sufficiently?  Some of your
proposal seems to be a fallout of not allowing exceptions, in my
opinion.  We should always use RAII (E.6) in any case.

> 
> Again, the above is by no means exhaustive; it only lists features which we
> propose should be completely banned.
> 
> 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."

> 
> Also, we do not feel strongly about stream operators, but would in general
> object to using it as the primary grounds to not use a particular library,
> as an
> example.

Again, this was not meant to be a total prohibition, unless it causes a
systemic concern.  Using streams for a one-off serial / deserial
function might be entirely reasonable (though we should use a
serialization library instead).  The concern was that logging is a
widely-used operation and the stream-style operators cause a big
number of function call locations which result in much larger code than
a direct call to a printf-style function.

Real-ish numbers here are ~10 instructions per item added to the log for
stream-style vs 10 + N instructions for printf-style.  I looked at a
random sample of our legacy code and about 5% of the LOC were log
statements, so this difference adds up fairly quickly.


[1] http://paulgraham.com/head.html
[2] https://github.com/openbmc/docs/blob/master/contributing.md
[3] https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md
[4] https://github.com/openbmc/sdbusplus/blob/master/sdbusplus/server/object.hpp#L63

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: C++ Feature Whitelist/Blacklist

[Brendan Higgins]

[-- Attachment #1: Type: text/plain, Size: 2038 bytes --]

>
> Hi Brendan
>
> First let me say thanks for your work formalizing processes and
> facilitating community
> participation by raising concerns, documenting things, your code
> contributions, etc.
> Thats all goodness.
>
> I think the lists below make sense 95% of the time.  Perhaps more.  If we
> have a formalized
> list such as the one below, and I find myself in the 5% situation, what
> does that mean?
>
> Maybe a better question for discussion is, could you elaborate on what
> advantages a community
> project with a set of non-negotiable rules would have over a project
> without such a set?
>
> thx - brad
>

Great question,

First off, what I consider a blacklist to be: I call it a black list to
distinguish it from the rest of the style guide purely for the purpose of
discussion; I do not plan on submitting this as part of a separate
document; they will be mixed in with the rest of the style guide.
Nevertheless, most rules and suggestions in a style guide are much weaker
than effectively removing a feature from a language. Things are usually
discouraged, but not _forbidden_. Naturally, a stronger rule requires more
discussion and will elicit a stronger reaction from someone who either
agrees with it or disagrees with it; that is the only reason I pulled it
out into a separate discussion.

What I mean when I say forbidden: I think there are exceptions to every
rule, but if a style guide says something is forbidden, the person who is
writing the code needs to document clearly why he is breaking convention
and the reviewers are to make clear that the exception is well explained
(and that they agree with it). So  I would not say they are non-negotiable;
they place a clear onus on the people writing code to document a very good
reason for doing something which makes it easier for reviewers; it also
makes it easier for people writing code because they know what they are
allowed to do and what they are not; so they know what to expect when time
for code review, saving review cycles and thus time.

Cheers!

[-- Attachment #2: Type: text/html, Size: 2926 bytes --]

Re: C++ Feature Whitelist/Blacklist

[Brendan Higgins]

[-- Attachment #1: Type: text/plain, Size: 21402 bytes --]

Summary
=======

To sum up the discussion below so far for everyone who is not Patrick and
me:
    * it sounds like there is some discussion to be had on how strong a
prohibition should be.
    * it seems that Patrick and I have general agreement on (just not
necessarily exact
      strength of rule):
        - Variables of class type with static storage duration (including
constants),
          plain old data types (POD) are allowed
        - Implicit conversion constructors and operators
        - Multiple Inheritance (I made a typo that I think got construed
that I did not want any
          multiple inheritance, including interfaces, when I wanted to only
allow multiple
          inheritance of interfaces)
    * issues still under discussion
        - Non-const-non-r-value references
        - Exceptions

This is not to be construed that any of the above items are closed or
discussion has concluded; this is only intended as a convenience for others
who are following the conversation that I think Patrick and I had come to
an agreement on something or that we are continuing to discuss it. I
certainly have no formal authority in the project and only wish to make it
as easy as possible for people to follow an important discussion. I would
encourage anyone who feels strongly about anything discussed so far or not
discussed so far to share his or her view points on the matter.

Intro
------

With regards to what you said about philosophy about style guides and the
Google style guide in particular; I do not disagree.

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.


I agree in principle; we should not blindly reuse a style guide that was
intended for a completely different type of project. However, I do think
that some sort of style guide is essential. The intention of the project is
to have as many people as possible contributing; thus it should be easy for
them to contribute and not get disheartened; I think a style guide is
essential in creating this environment because it sets up expectations for
what code should look like. Code review is inevitably the place where a
newcomers learn a great deal of how a project works and how to contribute
happens, but having a set of basic expectations reduces the pain on all
sides for the first few code reviews; this is important for not burning out
first time contributors.

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


I do not want to go too far into this because I do not want to have an
extended conversation about microservices as it would be off topic;
however, I will say a microservice architecture does not protect against
bad coding practices and is not intended to do so. Being able to come up
with a good mental model for a project comes solely from good abstractions
and thus clean code. The goal should not be to make it possible for a small
number of people to specialize and understand a handful of repos, but to
allow for anyone to jump in and understand any part of the project with
only a modicum of effort; consistency is required to achieve this and
consistency is built on rules.

I do suspect that the majority of our "rules" will evolve around the
> DBus interfaces, which are fairly unique to our project.


Again, I agree in principle, but I would caution against relying too
heavily on concepts particular to a library or framework. I see nothing
wrong with a microservice architecture, but I do not really see that as
unique and would hope to avoid making it overly unique. In the sense that
these ideas are not new and we should try our best to keep it that way.
Also, yes I fully expect the rules by which we govern the project to change
over time, but that does not mean we should not make a style guide.

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


I agree entirely. I made a response to this effect to the email sent by
Brad (I forwarded the contents of his email to the list in my email this
morning). I used the terms whitelist and blacklist entirely as a convention
to refer to what we are currently discussing: the stronger rules that will
require more discussion and likely solicit stronger opinions; although
these rules are stronger than the other rules; I do not expect them to be
without exception (except for maybe exceptions ;-), I will get to that
later).

Re: General Code Philosophy
---------------------------------------

    1) Conciseness:
>         - Did you use far more lines of code to solve the problem than
>           necessary?  Those are lines we have to maintain.


I tend to disagree that this should be the most important thing. I think
"easy to read" is far more important than conciseness; it is true that it
may create more lines of code to maintain, but if they are easy to
understand, they will take less effort to maintain. Still, conciseness is
something that does have value; I would agree that if two solutions are
equally easy to read than the shorter one is probably better assuming there
are no other detractors to it, but I would also argue that it is actually
easier to read in that case.

    2) Modern, widely-accepted practices:
>         - This is already spelled out in the 'contributing' doc, having
>           priority over any specific style statements. [2]


I certainly cannot argue with that. Keeping concepts and practices
generally consistent with the greater programming community has a lot of
value.

    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?


This is a pretty tough one; although no one can dispute that performance at
some level is always critically important especially when resources are
scarce, it is also true that writing good code in any context is always a
balance of performance and development cost.

Quantifying "code smell" into a set of non-ambiguous rules is a hard
> problem


Absolutely, and that is not my aim. Style guides are tools to make it
easier to have conversations about code. They set the base expectations and
provide a structure for people to discuss code in the context of. If one
disagrees with a practice, she has a document that she can make arguments
against (it is hard to argue with a loose set of practices and thus hard to
change them).

I would
> prefer we start with industry recognized experts opinion[3] and then
> document deviations specific to OpenBMC as they come up.


I agree with trying to follow generally accepted practices and it seems
like the CppCoreGuidelines seem to encode that pretty well, but it is not
really the same as a style guide and I see no reason why that precludes us
from writing one. I would, however, caution against citing the opinions of
"industry recognized experts" as sole justification for something. If the
opinion itself is cited with good reasoning/good supported evidence that's
fine, but it is important not to discount someone because he disagrees with
someone important. I am not saying you did that anywhere, all of the
arguments you made below either make clear arguments that stand on their
own or cite well thought out self-contained arguments, just something to
keep in mind.

Re: Variables of class type with static storage duration (including
constants)
-----------------------------------------------------------------------------------------------------

This proposal is a key example on why it is hard to quantify these types
> of rules.  Everyone agrees with a general principle of "reducing
> globals", but as spelled out this proposal would prohibit
> initializer-list construction, std::string, and the pattern within our
> IPMI plugins of "register on load".  These are all reasonable exceptions
> that require little justification in my mind.


It sounds like we agree in principle. My proposal would prohibit
std::string constants, but it would not prevent their general use.
Initializer list construction would not be prohibited for c-style arrays
and not its use in general; you would not be able to use it for
constructing variables of class type with static storage, but it outright
prohibits that anyway. As written currently, yes the ipmid plugin
registration code would not conform, but it would be possible to write it
in a way that does conform; this is actually something I am working on with
my changes to ipmid, but that is a discussion for elsewhere. (I would like
to be clear that this does not discourage global pointers, but in general
these also would require a good justification).

Re: Implicit conversion constructors and operators
-------------------------------------------------------------------

This would prohibit 'unique_ptr<T>::operator bool()' and
> 'string::string(const char*)', which are part of the STL and exist
> for concise syntax.  I do agree with this in most cases but there are
> cases where they both obvious and crisper.  A non-obvious conversion
> should be prohibited.


Maybe I should have called out more clearly that I do not plan on
prohibiting widely used parts of the styleguide that do these types of
things. In anycase, the smart pointers are special in that they maintain
the API that exists for raw pointers and it is common practice for people
to check for a nullptr by simply treating it as a boolean value.
Nevertheless, the vast majority of custom APIs should not use this feature,
and you seem to agree with this point in general anyway.

Re: Multiple Inheritance
--------------------------------

Composition is fairly common practice in C++ which uses multiple
> inheritance.  The sdbusplus bindings, as an example, have a specific
> template to aid in the composition of N dbus interfaces into a single
> C++ class. [4]


Your argument seems to be arguing for allowing multiple inheritance *of
interfaces*, which I meant to allow. I said "unless at most one of the
parent classes is an interface i.e. only has pure virtual methods", it
should have read "unless at most one of the parent classes is *not* an
interface i.e. only has pure virtual methods". You can inherit from as many
interfaces as you want, but only one (or no) non-interface. Technically,
your sdbusplus bindings are not interfaces, but I could see an exception
for that, because they are fairly special constructs. *It quacks like an
interface*, from the standpoint of an implementor. Also, the CCG sections
you cited agree with this standpoint. C.135 specifically calls out
interfaces and C.136 seemed to want to discourage the behaviour in the
reasoning section.

Re: Non-const-non-r-value references
---------------------------------------------------

I find this especially unnecessary, not recommended anywhere outside of
> the GSG, and having a crippling effect to the language.  C++ is not a
> procedural / OO programming language; it is also a functional
> programming language.  The bulk of the STL is to facilitate a
> functional programming style and many of the C++11 features (lambdas,
> constexpr functions, etc.) take large inspiration from FP.  Eliminating
> l-value reference makes FP techniques exceptionally harder and
> syntactically clumsier.


Again, I explicitly allowed use where necessary to maintain consistency our
utilize the STL. But in general, const references, r-value references, and
pointers together fulfill the same purposes and make it easier to
understand what the code is doing. Const reference implies nothing will
happen to the data, it will not (outside of exceptional circumstances)
store that reference and will not modify it; basically it stays in the
function. No special semantics are required when calling the function
because there is nothing to consider. R-value reference implies that the
data will be consumed, you do not own it anymore and it does it in a
beautifully explicit way, std::move, which means that you know the data
does not belong to you anymore when you call the function. Providing a
pointer requires you to either be working with a pointer which is clear
because it has different semantics, or you have to pass a pointer
explicitly; either way, you know that the function is free to edit what you
passed in; it should not store it because it does not own it (otherwise use
a smart pointer), but is free to edit it in the context of the function
call. I do not see it standing in the way of FP at all, in FP you are
usually dealing with immutable data types which you should use as much as
possible i.e. const references, in the case that is expensive you have
r-value references and pointers. Although, the CCG does say it is allowed,
it admits that non-const references can lead to bugs see the note under
F.17.

A very common recommendation in "Exceptional Modern C++" is to define a
> function like:
>     template <typename T> foo(T&&)
> This function operates on both l-value and r-value references
> ("universal references" as Meyers calls them).


I think I am missing something here. l-value and r-value references are two
different things and are treated differently by the compiler. I tried the
following:

int add(int&& l, int&& r) {
  return l + r;
}

int main() {
  int l = 1;
  int r = 2;
  std::cout << "add(1, 2) = " << add(l, r) << std::endl;
  return 0;
}

and got the following compile error:

test.cc:10:42: error: cannot bind ‘int’ lvalue to ‘int&&’
   std::cout << "add(1, 2) = " << add(l, r) << std::endl;

which is precisely what I expected to happen. I think I am missing part of
your example.

Re: Exceptions
--------------------

This was the "old way" in C++ and the standards body specifically
> deprecated what you are suggesting.  I'm sure you could read their
> rationale.


Sure, I would be happy to. I tried googling around and the only recent
update I could find relating to how the compiler treats exceptions was the
addition of noexcept. Could you share this?

In GCC, at least, function attributes can only be defined as part of
> declaration and not definition.  That means you would have to forward
> declare every function that returns your error type.


That's true. Clang allows you to put the attribute on the type definition,
I forgot gcc does not allow that. Still, if the function was part of a
public interface, you would have to forward declare anyway. In anycase, my
point was that there are alternatives to exceptions where the compiler will
actually bug you if you ignore it.

In every large project I have been involved with, an integer quickly
> becomes insufficient to express "an error".  The projects all make their
> own "error object".  According to Titus' talk, Google has one of these
> internally as well.  So that means we're now talking about passing
> around a heavy object by value or a pointer to one (which is always at
> risk of leaking)?  What does this error look like?


Also true, hypothetically we would use some sort of error type. Probably an
error enum and an optional string message. Google has an open source one we
could use, or we could write our own. In any case, it does not need to be
heavy weight, as long as the field list is small the object will be small.
For user defined types that fit inside of a few machine types, gcc and
clang will frequently pass them via register instead of stack. In any case,
I would expect the error object to be returned so it would be subject to
return value optimization.

Having a error return type likely eliminates any RVO and again plays a
> huge diminishing role in using functional programming methods.  The two
> approaches for output variables become:
>     1) Fill-in out parameter reference.  [anti-FP]
>     2) Return a tuple<error, real_return>.
>

The approach we use in this case is called StatusOr<T>; it wraps the type
being returned and the associated Status object (what we call the error
object). This is actually a really common pattern outside of C++ and
parallels the error monad type found in a lot of functional programming
languages (we do not actually support the fmap operation, but the usage
pattern is semantically equivalent).


> >         - Exceptions are not typed
>
> Exceptions are strongly typed in the exact same type system as any
> reference.  I don't know what this statement means.


Sorry, what I meant by this is exceptions are unchecked in C++ so you can
throw any type; hence, there is no way to enforce what kinds of exceptions
are thrown and that those exceptions are caught in functions that do not
forward them as in Java.


> Again this is the "lowest common denominator" argument, which I do not

accept.  We could make the same argument with our legacy codebase, which

I can assure you in this problem domain is much bigger than yours.  We

are making a concerted effort to not directly port any of our code we

are contributing to OpenBMC but to make sure it is modernized and

follows the practices with the rest of the codebase.


> Any code you are planning to port would either need to go though the

same vetting process as a clean contribution (for acceptance into an

openbmc tree) or would be maintained in your own repositories.  If it is

something that is maintained in your own repositories, the only place

exceptions could be introduced is when you call base OpenBMC utilities,

like sdbusplus.  It would not be unreasonable to "catch" all exceptions

at the API level into base utilities and turn them into a return code.

(And in fact, I highly suspect you already have these abstracted out due

to your Google Mock test framework).


> Since the majority of the repositories have self-contained programs that

expose DBus interfaces, what those programs do internally has no impact

on your ported programs that are interacting with those DBus interfaces.

DBus itself already has a DBus-Error type, which is distinct from method

return values, that you are free to turn into a return code instead of

an exception.


Fair enough, this is admittedly not a great argument, but in the case of an
even split, it is of minor benefit.

CppCoreGuidelines C.44 and C.50 cover this sufficiently?  Some of your
> proposal seems to be a fallout of not allowing exceptions, in my
> opinion.  We should always use RAII (E.6) in any case.


Fair enough.

Re: 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."


I would agree that class hierarchies should be well designed and thought
out, but I could say the same thing about every bit of code that is
written, especially when it is part of the core structure or the externally
exposed API. I would, however, argue that "justification required" does
imply that the behavior is discouraged; at least my definition of
discouraged in a style guide means that a justification is required to use
it. Interfaces, and thus virtual functions are required for good unit
testing and mocking, so as long as that is not being restricted in that
sense I do not have any complaints, I am not sure where that falls on the
spectrum of "requires justification".

Again, this was not meant to be a total prohibition, unless it causes a
> systemic concern.  Using streams for a one-off serial / deserial
> function might be entirely reasonable (though we should use a
> serialization library instead).  The concern was that logging is a
> widely-used operation and the stream-style operators cause a big
> number of function call locations which result in much larger code than
> a direct call to a printf-style function.


Fair enough, I tried to avoid characterizing your argument because I felt
it was unfair and that you should be allowed to present it yourself.

Cheers

[-- Attachment #2: Type: text/html, Size: 36128 bytes --]

Re: C++ Feature Whitelist/Blacklist

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 18864 bytes --]

On Wed, Nov 02, 2016 at 05:02:27PM -0700, Brendan Higgins wrote:
>     * it seems that Patrick and I have general agreement on (just not
> necessarily exact
>       strength of rule):
>         - Variables of class type with static storage duration (including
> constants),
>           plain old data types (POD) are allowed

I'm not convinced we have agreement on this one.

> > I would
> > prefer we start with industry recognized experts opinion[3] and then
> > document deviations specific to OpenBMC as they come up.
> 
> 
> I agree with trying to follow generally accepted practices and it seems
> like the CppCoreGuidelines seem to encode that pretty well, but it is not
> really the same as a style guide and I see no reason why that precludes us
> from writing one. 

There is obviously a major cultural difference here (between Google and
"everyone else").  What Google calls a style guide is a combination of
what others call two different documents: a coding style guide and a
coding conventions (or guidelines) document.  Style guides describe the
location of spaces, names of variables, etc.  Coding guidelines give you
the "Do's and Don'ts".  The QT project, for example, has called them
exactly that: "coding style"[1] and "coding conventions"[2].

I honestly think that the difference between the two is partially to
blame for why we have been talking past each other previously in the
need for a "style guide".

> I would, however, caution against citing the opinions of
> "industry recognized experts" as sole justification for something. If the
> opinion itself is cited with good reasoning/good supported evidence that's
> fine, but it is important not to discount someone because he disagrees with
> someone important. I am not saying you did that anywhere, all of the
> arguments you made below either make clear arguments that stand on their
> own or cite well thought out self-contained arguments, just something to
> keep in mind.

Also be aware that this is not some weak "appeal to authority" here.
We're talking about Bjorn and Herb.  In case you are not familiar, they
are the creator of C++ and the convener of the standards body
respectively.  Discounting their opinion is like saying Linus doesn't
know what he is talking about.  The balance is surely tipped in their
favor coming into any discussion and you better have some pretty
significant argument against it.

> 
> Re: Variables of class type with static storage duration (including
> constants)
> -----------------------------------------------------------------------------------------------------
> 
> This proposal is a key example on why it is hard to quantify these types
> > of rules.  Everyone agrees with a general principle of "reducing
> > globals", but as spelled out this proposal would prohibit
> > initializer-list construction, std::string, and the pattern within our
> > IPMI plugins of "register on load".  These are all reasonable exceptions
> > that require little justification in my mind.
> 
> 
> It sounds like we agree in principle. My proposal would prohibit
> std::string constants, but it would not prevent their general use.
> Initializer list construction would not be prohibited for c-style arrays
> and not its use in general; you would not be able to use it for
> constructing variables of class type with static storage, but it outright
> prohibits that anyway. As written currently, yes the ipmid plugin
> registration code would not conform, but it would be possible to write it
> in a way that does conform; this is actually something I am working on with
> my changes to ipmid, but that is a discussion for elsewhere. (I would like
> to be clear that this does not discourage global pointers, but in general
> these also would require a good justification).

The only time there is unspecified behavior in the standard is when you
are using a symbol defined in another compilation unit in your own
compilation unit during the initialization phase of your object.

I do not understand why you would want to prohibit std::string constants
and any other initializer-list constructed data structures when they are
not the issue.  We are often generating a data structure from machine
description files and using the initializer-list syntax to prime those
structures.  Having to do it outside an initializer-list requires a
special "globalDataInit()" function which is a needless and less
efficient hoop to jump through.

In my view, I.22 already has everything covered.  It also has a better
description of the problem areas in the "Enforcement" heading
(non-constexpr functions, extern objects).

> 
> Re: Implicit conversion constructors and operators
> -------------------------------------------------------------------
> 
> This would prohibit 'unique_ptr<T>::operator bool()' and
> > 'string::string(const char*)', which are part of the STL and exist
> > for concise syntax.  I do agree with this in most cases but there are
> > cases where they both obvious and crisper.  A non-obvious conversion
> > should be prohibited.
> 
> 
> Maybe I should have called out more clearly that I do not plan on
> prohibiting widely used parts of the styleguide that do these types of
> things. In anycase, the smart pointers are special in that they maintain
> the API that exists for raw pointers and it is common practice for people
> to check for a nullptr by simply treating it as a boolean value.
> Nevertheless, the vast majority of custom APIs should not use this feature,
> and you seem to agree with this point in general anyway.

I think I agree, it just feels odd to point out.  Who writes an operator
overload "just because"?  Everyone who writes one thinks that the
rationale for it is obvious.

> Re: Multiple Inheritance
> --------------------------------
> 
> > Composition is fairly common practice in C++ which uses multiple
> > inheritance.  The sdbusplus bindings, as an example, have a specific
> > template to aid in the composition of N dbus interfaces into a single
> > C++ class. [4]
> 
> 
> Your argument seems to be arguing for allowing multiple inheritance *of
> interfaces*, which I meant to allow. I said "unless at most one of the
> parent classes is an interface i.e. only has pure virtual methods", it
> should have read "unless at most one of the parent classes is *not* an
> interface i.e. only has pure virtual methods". You can inherit from as many
> interfaces as you want, but only one (or no) non-interface. Technically,
> your sdbusplus bindings are not interfaces, but I could see an exception
> for that, because they are fairly special constructs. *It quacks like an
> interface*, from the standpoint of an implementor. Also, the CCG sections
> you cited agree with this standpoint. C.135 specifically calls out
> interfaces and C.136 seemed to want to discourage the behaviour in the
> reasoning section.

Right.  There is an obvious overload of the word "interface" in this
case.  Someone reviewing what you wrote is coming at it with the Java
language "interface" mindset: a pure-virtual class.  The DBus Interface
bindings are clearly not pure-virtual but they are "interface-like".

Again, multiple inheritance isn't something that I've seen people even
attempt typically unless they know what they are doing.  So it seems odd
to point it out but then exempt "obvious" usage.

> Re: Non-const-non-r-value references
> ---------------------------------------------------
> 
> > I find this especially unnecessary, not recommended anywhere outside of
> > the GSG, and having a crippling effect to the language.  C++ is not a
> > procedural / OO programming language; it is also a functional
> > programming language.  The bulk of the STL is to facilitate a
> > functional programming style and many of the C++11 features (lambdas,
> > constexpr functions, etc.) take large inspiration from FP.  Eliminating
> > l-value reference makes FP techniques exceptionally harder and
> > syntactically clumsier.
> 
> 
> Again, I explicitly allowed use where necessary to maintain consistency our
> utilize the STL. 

The logical conclusion of this exemption is to actively encourage use of
non-const l-value references, which is surely not what you intend
(though, I do).

    - As an author of an API, I do not know how users are going to
      organize their own data.
    - As an author of an API, I should do nothing to preclude reasonable
      programming patterns.
    - All container iterators work by default on l-value references.
    - Functors and lambdas to the STL algorithms work easiest on l-value
      references.
    - Therefore, I should conclude that my users could have a collection
      of things and could desire to use functional programming style,
      and I should make life easier for them by taking an l-value
      reference.

> But in general, const references, r-value references, and
> pointers together fulfill the same purposes and make it easier to
> understand what the code is doing. Const reference implies nothing will
> happen to the data, it will not (outside of exceptional circumstances)
> store that reference and will not modify it; basically it stays in the
> function. No special semantics are required when calling the function
> because there is nothing to consider. R-value reference implies that the
> data will be consumed, you do not own it anymore and it does it in a
> beautifully explicit way, std::move, which means that you know the data
> does not belong to you anymore when you call the function. Providing a
> pointer requires you to either be working with a pointer which is clear
> because it has different semantics, or you have to pass a pointer
> explicitly; either way, you know that the function is free to edit what you
> passed in; it should not store it because it does not own it (otherwise use
> a smart pointer), but is free to edit it in the context of the function
> call. 

I understand what you and the GSG are saying.  I just disagree with it.

In C, everything is a pointer.  I could just write all of my code to not
use references at all and instead use 'T*' and 'const T*'.  You now
still have no idea when you read 'foo(&bar, &baz)'.

In C++ there is always const_cast, so even if something is passed by
const it _could_ be manipulated.  (Not to say this is good practice in
any way.  See ES.50).

It is a matter of public record[3] that I have reviewed on roughly
50-100kLOC a year for the last 5 years and I still don't buy into this
reviewer argument.  If someone is calling an API and I don't know what
that API is doing, I don't give them a +1 no matter how "obvious" it may
seem.

The rough summary of my viewpoint is:
    1) This syntax simply lulls you into a false sense of security as
       the reviewer.
    2) This syntax greatly diminishes aspects of modern C++ practices.
    3) This syntax is not recommended anywhere outside of Google.

> I do not see it standing in the way of FP at all, in FP you are
> usually dealing with immutable data types which you should use as much as
> possible i.e. const references, in the case that is expensive you have
> r-value references and pointers. 

FP style does not always mean immutable.  A toy example:

    vector<int> stuff = ...;
    auto times2 = [](auto& x) { x *= 2; }
    for_each(stuff, times2);

> Although, the CCG does say it is allowed,
> it admits that non-const references can lead to bugs see the note under
> F.17.

It is a pretty weak argument for you to use a sectioned titled "For
'in-out' parameters, pass by reference to non-const" as evidence.

The "error" they are talking about is a reassertion of F.16: "For 'in'
parameters, pass ... by reference to const".  Non-const implies it could
be changed.

> 
> A very common recommendation in "Exceptional Modern C++" is to define a
> > function like:
> >     template <typename T> foo(T&&)
> > This function operates on both l-value and r-value references
> > ("universal references" as Meyers calls them).
> 
> 
> I think I am missing something here. l-value and r-value references are two
> different things and are treated differently by the compiler. I tried the
> following:
> 
> int add(int&& l, int&& r) {
>   return l + r;
> }
> 
> int main() {
>   int l = 1;
>   int r = 2;
>   std::cout << "add(1, 2) = " << add(l, r) << std::endl;
>   return 0;
> }
> 
> and got the following compile error:
> 
> test.cc:10:42: error: cannot bind ‘int’ lvalue to ‘int&&’
>    std::cout << "add(1, 2) = " << add(l, r) << std::endl;
> 
> which is precisely what I expected to happen. I think I am missing part of
> your example.

What I wrote and what you tested are two entirely different things.  I
wrote a template; you tested a non-template.  Templates and autos fall
under type-deduction rules.  Read up on those and reference collapsing.

foo(10) => foo(int&&)
foo(x) => foo(int& &&) => foo(int&)

> 
> Re: Exceptions
> --------------------
> 
> > This was the "old way" in C++ and the standards body specifically
> > deprecated what you are suggesting.  I'm sure you could read their
> > rationale.
> 
> 
> Sure, I would be happy to. I tried googling around and the only recent
> update I could find relating to how the compiler treats exceptions was the
> addition of noexcept. Could you share this?

See deprecation of 'throw(typeid, ...)'. : 
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3051.html

(I haven't actually read the reasoning myself, I just know it is how it
was previously done and is now deprecated.)

> 
> > In GCC, at least, function attributes can only be defined as part of
> > declaration and not definition.  That means you would have to forward
> > declare every function that returns your error type.
> 
> 
> That's true. Clang allows you to put the attribute on the type definition,
> I forgot gcc does not allow that. Still, if the function was part of a
> public interface, you would have to forward declare anyway. In anycase, my
> point was that there are alternatives to exceptions where the compiler will
> actually bug you if you ignore it.
> 
> > In every large project I have been involved with, an integer quickly
> > becomes insufficient to express "an error".  The projects all make their
> > own "error object".  According to Titus' talk, Google has one of these
> > internally as well.  So that means we're now talking about passing
> > around a heavy object by value or a pointer to one (which is always at
> > risk of leaking)?  What does this error look like?
> 
> 
> Also true, hypothetically we would use some sort of error type. Probably an
> error enum and an optional string message. Google has an open source one we
> could use, or we could write our own. In any case, it does not need to be
> heavy weight, as long as the field list is small the object will be small.
> For user defined types that fit inside of a few machine types, gcc and
> clang will frequently pass them via register instead of stack. In any case,
> I would expect the error object to be returned so it would be subject to
> return value optimization.
> 
> > Having a error return type likely eliminates any RVO and again plays a
> > huge diminishing role in using functional programming methods.  The two
> > approaches for output variables become:
> >     1) Fill-in out parameter reference.  [anti-FP]
> >     2) Return a tuple<error, real_return>.
> >
> 
> The approach we use in this case is called StatusOr<T>; it wraps the type
> being returned and the associated Status object (what we call the error
> object). This is actually a really common pattern outside of C++ and
> parallels the error monad type found in a lot of functional programming
> languages (we do not actually support the fmap operation, but the usage
> pattern is semantically equivalent).

StatusOr requires everything you are returning to have a [public] default
constructor available to StatusOr and copy operations.

> 
> 
> > >         - Exceptions are not typed
> >
> > Exceptions are strongly typed in the exact same type system as any
> > reference.  I don't know what this statement means.
> 
> 
> Sorry, what I meant by this is exceptions are unchecked in C++ so you can
> throw any type; hence, there is no way to enforce what kinds of exceptions
> are thrown and that those exceptions are caught in functions that do not
> forward them as in Java.

So we are capable of writing a guideline that we should not use globals
but we are incapable of writing a guidelines that requires all
exceptions being derived from std::exception?  Oh, wait... E.14 already
implies this.

> Re: Virtual Functions
> ----------------------------

> Interfaces, and thus virtual functions are required for good unit
> testing and mocking, 

I agree with the statement "Interfaces are required for good unit
testing".  I do not agree with the "and thus virtual functions".  You
can also solve this at link time with two alternative implementations of
the class.  

I'm frankly amazed G-Mock doesn't allow this.  The only rationale I can
come up with is that "big" processors tend to have branch predictors on
virtual function tables while "small" processors do not, so for the
environments Google typically targets there really isn't any thought
put into the cost of virtual.

Though, marking everything virtual also eliminates any sort of 'inline',
so I am somewhat perplexed even on that aspect as the basis.


Back to Big Picture
----------------------------

I do feel we are seriously missing a broader philosophical question.  In
what ways do you feel the CppCoreGuidelines do not address all of your
coding guidelines suggestions?  Rather than write an entirely new
document why would we not simply point to that and then extend it if we
feel it is insufficient in a few key areas.  As you saw in my previous
email, nearly everything we agree on comes from the CCG already and
nearly everything we disagree on is a contradiction from the CCG.

I do not want to hear that the CCG is "too long".  If you take out the
references it is in the same order of magnitude as the GSG.  It is also
organized in a way where each section is a straight-forward statement to
be followed, so anyone unfamiliar or uninterested can simply read the
TOC to understand all of the rules (ex. "I.2 Avoid global variables.",
E.6 Use RAII to prevent leaks").


[1] http://wiki.qt.io/Qt_Coding_Style
[2] http://wiki.qt.io/Coding_Conventions
[3] https://github.com/open-power/hostboot
-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: C++ Feature Whitelist/Blacklist

[Nancy Yuen]

[-- Attachment #1: Type: text/plain, Size: 6079 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 7402 bytes --]

Re: C++ Feature Whitelist/Blacklist

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 2655 bytes --]

Nancy,

A few minor responses below.

On Fri, Nov 04, 2016 at 02:46:42AM -0700, Nancy Yuen wrote:
> 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.

What I meant by this was not that every contributor is expected to know
all of the code, but that the maintainer is likely to know the internal
APIs of it enough to be able to review contributions.  One of the main
points of contention is a proposal that "makes it easier to review" and
that is the part that makes more sense when you are talking about 1+
MLOC of interdependent code.  That particular point is less interesting
when you are talking about <50kLOC.

> Btw, Google's codebase is not monolithic.

I realize that was not likely the case, but those were the words Titus
used in his public talk.

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

I feel you have taken this one sentence very much out of context.

The first paragraph of the abstract says: 

    The C++ Core Guidelines are a collaborative effort led by Bjarne
    Stroustrup, much like the C++ language itself. They are the result of
    many person-years of discussion and design across a number of
    organizations. Their design encourages general applicability and broad
    adoption but they can be freely copied and modified to meet your
    organization's needs.

They are actively encouraging others to use this list.  The also state
in the abstract:

    The rules are designed to be supported by an analysis tool. Violations
    of rules will be flagged with references (or links) to the relevant
    rule. We do not expect you to memorize all the rules before trying to
    write code.

In fact, clang-tidy has a number of checks for these rules to encourage
their use.

The one sentence you quoted I read as "We are willing to change these
rules if evidence points to the contrary.  Please prove us wrong."  And,
in fact, the guidelines are "actively maintained" on Github and have
changed since their original introduction.

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: C++ Feature Whitelist/Blacklist

[Nancy Yuen]

[-- Attachment #1: Type: text/plain, Size: 22937 bytes --]

On Thu, Nov 3, 2016 at 5:03 AM, Patrick Williams <patrick@stwcx.xyz> wrote:
>
> On Wed, Nov 02, 2016 at 05:02:27PM -0700, Brendan Higgins wrote:
> >     * it seems that Patrick and I have general agreement on (just not
> > necessarily exact
> >       strength of rule):
> >         - Variables of class type with static storage duration
(including
> > constants),
> >           plain old data types (POD) are allowed
>
> I'm not convinced we have agreement on this one.
>
> > > I would
> > > prefer we start with industry recognized experts opinion[3] and then
> > > document deviations specific to OpenBMC as they come up.
> >
> >
> > I agree with trying to follow generally accepted practices and it seems
> > like the CppCoreGuidelines seem to encode that pretty well, but it is
not
> > really the same as a style guide and I see no reason why that precludes
us
> > from writing one.
>
> There is obviously a major cultural difference here (between Google and
> "everyone else").  What Google calls a style guide is a combination of
> what others call two different documents: a coding style guide and a
> coding conventions (or guidelines) document.  Style guides describe the
> location of spaces, names of variables, etc.  Coding guidelines give you
> the "Do's and Don'ts".  The QT project, for example, has called them
> exactly that: "coding style"[1] and "coding conventions"[2].
>
> I honestly think that the difference between the two is partially to
> blame for why we have been talking past each other previously in the
> need for a "style guide".
>

Agreed, and I thought we discussed this in our meetings last week.  What
we'd
like to define is more than just style.  I don't care what we call it
though.
A style guide, language guidelines, rules.  But having these conventions and
rules around helps programmers contribute, especially programmers who are
just
getting started with OpenBMC.

> > I would, however, caution against citing the opinions of
> > "industry recognized experts" as sole justification for something. If
the
> > opinion itself is cited with good reasoning/good supported evidence
that's
> > fine, but it is important not to discount someone because he disagrees
with
> > someone important. I am not saying you did that anywhere, all of the
> > arguments you made below either make clear arguments that stand on their
> > own or cite well thought out self-contained arguments, just something to
> > keep in mind.
>
> Also be aware that this is not some weak "appeal to authority" here.
> We're talking about Bjorn and Herb.  In case you are not familiar, they
> are the creator of C++ and the convener of the standards body
> respectively.  Discounting their opinion is like saying Linus doesn't
> know what he is talking about.  The balance is surely tipped in their
> favor coming into any discussion and you better have some pretty
> significant argument against it.
>

I'm not sure why sarcasm is necessary here.  It's not conducive to a
productive
discussion.

Yes, we're all familiar with Bjorn and Herb and their roles in the
standards and
the C++ language.  Doesn't mean they're never wrong.  There's no
discounting of
their opinion happening.  Just simply asking for good supported reasoning.
You
asked for the same when we brought up virtual functions.  Even in the
CppCoreGuidelines abstract, it states these rules may be too strict, may
need
changing or more exceptions.  It even asks for feedback:  "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."  It's a work in progress.
I don't know about you, but I've been taught never to just follow rules
unquestioningly, but to ask questions.

So far, it sounds like we mostly agree on the following:

> > Re: Implicit conversion constructors and operators
> > Re: Multiple Inheritance

I don't think it hurts to call it out though.  Bear in mind, this guide
should also
help for future developers contributing to OpenBMC, and their experience
levels
may vary.

> > Re: Variables of class type with static storage duration (including
> > constants)
> >
-----------------------------------------------------------------------------------------------------
> >
> > This proposal is a key example on why it is hard to quantify these types
> > > of rules.  Everyone agrees with a general principle of "reducing
> > > globals", but as spelled out this proposal would prohibit
> > > initializer-list construction, std::string, and the pattern within our
> > > IPMI plugins of "register on load".  These are all reasonable
exceptions
> > > that require little justification in my mind.
> >
> >
> > It sounds like we agree in principle. My proposal would prohibit
> > std::string constants, but it would not prevent their general use.
> > Initializer list construction would not be prohibited for c-style arrays
> > and not its use in general; you would not be able to use it for
> > constructing variables of class type with static storage, but it
outright
> > prohibits that anyway. As written currently, yes the ipmid plugin
> > registration code would not conform, but it would be possible to write
it
> > in a way that does conform; this is actually something I am working on
with
> > my changes to ipmid, but that is a discussion for elsewhere. (I would
like
> > to be clear that this does not discourage global pointers, but in
general
> > these also would require a good justification).
>
> The only time there is unspecified behavior in the standard is when you
> are using a symbol defined in another compilation unit in your own
> compilation unit during the initialization phase of your object.
>

Are you saying this won't happen or happens infrequently?  Or that
developers won't accidentally write code that triggers this?

> I do not understand why you would want to prohibit std::string constants
> and any other initializer-list constructed data structures when they are
> not the issue.  We are often generating a data structure from machine
> description files and using the initializer-list syntax to prime those
> structures.  Having to do it outside an initializer-list requires a
> special "globalDataInit()" function which is a needless and less
> efficient hoop to jump through.

Initialize-list constructed data structures are not prohibited in general,
only for class types with static storage.

>
> In my view, I.22 already has everything covered.  It also has a better
> description of the problem areas in the "Enforcement" heading
> (non-constexpr functions, extern objects).
>
> >
> > Re: Implicit conversion constructors and operators
> > -------------------------------------------------------------------
> >
> > This would prohibit 'unique_ptr<T>::operator bool()' and
> > > 'string::string(const char*)', which are part of the STL and exist
> > > for concise syntax.  I do agree with this in most cases but there are
> > > cases where they both obvious and crisper.  A non-obvious conversion
> > > should be prohibited.
> >
> >
> > Maybe I should have called out more clearly that I do not plan on
> > prohibiting widely used parts of the styleguide that do these types of
> > things. In anycase, the smart pointers are special in that they maintain
> > the API that exists for raw pointers and it is common practice for
people
> > to check for a nullptr by simply treating it as a boolean value.
> > Nevertheless, the vast majority of custom APIs should not use this
feature,
> > and you seem to agree with this point in general anyway.
>
> I think I agree, it just feels odd to point out.  Who writes an operator
> overload "just because"?  Everyone who writes one thinks that the
> rationale for it is obvious.
>
> > Re: Multiple Inheritance
> > --------------------------------
> >
> > > Composition is fairly common practice in C++ which uses multiple
> > > inheritance.  The sdbusplus bindings, as an example, have a specific
> > > template to aid in the composition of N dbus interfaces into a single
> > > C++ class. [4]
> >
> >
> > Your argument seems to be arguing for allowing multiple inheritance *of
> > interfaces*, which I meant to allow. I said "unless at most one of the
> > parent classes is an interface i.e. only has pure virtual methods", it
> > should have read "unless at most one of the parent classes is *not* an
> > interface i.e. only has pure virtual methods". You can inherit from as
many
> > interfaces as you want, but only one (or no) non-interface. Technically,
> > your sdbusplus bindings are not interfaces, but I could see an exception
> > for that, because they are fairly special constructs. *It quacks like an
> > interface*, from the standpoint of an implementor. Also, the CCG
sections
> > you cited agree with this standpoint. C.135 specifically calls out
> > interfaces and C.136 seemed to want to discourage the behaviour in the
> > reasoning section.
>
> Right.  There is an obvious overload of the word "interface" in this
> case.  Someone reviewing what you wrote is coming at it with the Java
> language "interface" mindset: a pure-virtual class.  The DBus Interface
> bindings are clearly not pure-virtual but they are "interface-like".
>
> Again, multiple inheritance isn't something that I've seen people even
> attempt typically unless they know what they are doing.  So it seems odd
> to point it out but then exempt "obvious" usage.
>
> > Re: Non-const-non-r-value references
> > ---------------------------------------------------
> >
> > > I find this especially unnecessary, not recommended anywhere outside
of
> > > the GSG, and having a crippling effect to the language.  C++ is not a
> > > procedural / OO programming language; it is also a functional
> > > programming language.  The bulk of the STL is to facilitate a
> > > functional programming style and many of the C++11 features (lambdas,
> > > constexpr functions, etc.) take large inspiration from FP.
Eliminating
> > > l-value reference makes FP techniques exceptionally harder and
> > > syntactically clumsier.
> >
> >
> > Again, I explicitly allowed use where necessary to maintain consistency
our
> > utilize the STL.
>
> The logical conclusion of this exemption is to actively encourage use of
> non-const l-value references, which is surely not what you intend
> (though, I do).
>
>     - As an author of an API, I do not know how users are going to
>       organize their own data.
>     - As an author of an API, I should do nothing to preclude reasonable
>       programming patterns.
>     - All container iterators work by default on l-value references.
>     - Functors and lambdas to the STL algorithms work easiest on l-value
>       references.
>     - Therefore, I should conclude that my users could have a collection
>       of things and could desire to use functional programming style,
>       and I should make life easier for them by taking an l-value
>       reference.
>
> > But in general, const references, r-value references, and
> > pointers together fulfill the same purposes and make it easier to
> > understand what the code is doing. Const reference implies nothing will
> > happen to the data, it will not (outside of exceptional circumstances)
> > store that reference and will not modify it; basically it stays in the
> > function. No special semantics are required when calling the function
> > because there is nothing to consider. R-value reference implies that the
> > data will be consumed, you do not own it anymore and it does it in a
> > beautifully explicit way, std::move, which means that you know the data
> > does not belong to you anymore when you call the function. Providing a
> > pointer requires you to either be working with a pointer which is clear
> > because it has different semantics, or you have to pass a pointer
> > explicitly; either way, you know that the function is free to edit what
you
> > passed in; it should not store it because it does not own it (otherwise
use
> > a smart pointer), but is free to edit it in the context of the function
> > call.
>
> I understand what you and the GSG are saying.  I just disagree with it.
>
> In C, everything is a pointer.  I could just write all of my code to not
> use references at all and instead use 'T*' and 'const T*'.  You now
> still have no idea when you read 'foo(&bar, &baz)'.
>
> In C++ there is always const_cast, so even if something is passed by
> const it _could_ be manipulated.  (Not to say this is good practice in
> any way.  See ES.50).
>
> It is a matter of public record[3] that I have reviewed on roughly
> 50-100kLOC a year for the last 5 years and I still don't buy into this
> reviewer argument.  If someone is calling an API and I don't know what
> that API is doing, I don't give them a +1 no matter how "obvious" it may
> seem.
>
> The rough summary of my viewpoint is:
>     1) This syntax simply lulls you into a false sense of security as
>        the reviewer.
>     2) This syntax greatly diminishes aspects of modern C++ practices.
>     3) This syntax is not recommended anywhere outside of Google.
>
> > I do not see it standing in the way of FP at all, in FP you are
> > usually dealing with immutable data types which you should use as much
as
> > possible i.e. const references, in the case that is expensive you have
> > r-value references and pointers.
>
> FP style does not always mean immutable.  A toy example:
>
>     vector<int> stuff = ...;
>     auto times2 = [](auto& x) { x *= 2; }
>     for_each(stuff, times2);
>
> > Although, the CCG does say it is allowed,
> > it admits that non-const references can lead to bugs see the note under
> > F.17.
>
> It is a pretty weak argument for you to use a sectioned titled "For
> 'in-out' parameters, pass by reference to non-const" as evidence.
>
> The "error" they are talking about is a reassertion of F.16: "For 'in'
> parameters, pass ... by reference to const".  Non-const implies it could
> be changed.
>
> >
> > A very common recommendation in "Exceptional Modern C++" is to define a
> > > function like:
> > >     template <typename T> foo(T&&)
> > > This function operates on both l-value and r-value references
> > > ("universal references" as Meyers calls them).
> >
> >
> > I think I am missing something here. l-value and r-value references are
two
> > different things and are treated differently by the compiler. I tried
the
> > following:
> >
> > int add(int&& l, int&& r) {
> >   return l + r;
> > }
> >
> > int main() {
> >   int l = 1;
> >   int r = 2;
> >   std::cout << "add(1, 2) = " << add(l, r) << std::endl;
> >   return 0;
> > }
> >
> > and got the following compile error:
> >
> > test.cc:10:42: error: cannot bind ‘int’ lvalue to ‘int&&’
> >    std::cout << "add(1, 2) = " << add(l, r) << std::endl;
> >
> > which is precisely what I expected to happen. I think I am missing part
of
> > your example.
>
> What I wrote and what you tested are two entirely different things.  I
> wrote a template; you tested a non-template.  Templates and autos fall
> under type-deduction rules.  Read up on those and reference collapsing.
>
> foo(10) => foo(int&&)
> foo(x) => foo(int& &&) => foo(int&)
>
> >
> > Re: Exceptions
> > --------------------
> >
> > > This was the "old way" in C++ and the standards body specifically
> > > deprecated what you are suggesting.  I'm sure you could read their
> > > rationale.
> >
> >
> > Sure, I would be happy to. I tried googling around and the only recent
> > update I could find relating to how the compiler treats exceptions was
the
> > addition of noexcept. Could you share this?
>
> See deprecation of 'throw(typeid, ...)'. :
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3051.html
>
> (I haven't actually read the reasoning myself, I just know it is how it
> was previously done and is now deprecated.)
>
> >
> > > In GCC, at least, function attributes can only be defined as part of
> > > declaration and not definition.  That means you would have to forward
> > > declare every function that returns your error type.
> >
> >
> > That's true. Clang allows you to put the attribute on the type
definition,
> > I forgot gcc does not allow that. Still, if the function was part of a
> > public interface, you would have to forward declare anyway. In anycase,
my
> > point was that there are alternatives to exceptions where the compiler
will
> > actually bug you if you ignore it.
> >
> > > In every large project I have been involved with, an integer quickly
> > > becomes insufficient to express "an error".  The projects all make
their
> > > own "error object".  According to Titus' talk, Google has one of these
> > > internally as well.  So that means we're now talking about passing
> > > around a heavy object by value or a pointer to one (which is always at
> > > risk of leaking)?  What does this error look like?
> >
> >
> > Also true, hypothetically we would use some sort of error type.
Probably an
> > error enum and an optional string message. Google has an open source
one we
> > could use, or we could write our own. In any case, it does not need to
be
> > heavy weight, as long as the field list is small the object will be
small.
> > For user defined types that fit inside of a few machine types, gcc and
> > clang will frequently pass them via register instead of stack. In any
case,
> > I would expect the error object to be returned so it would be subject to
> > return value optimization.
> >
> > > Having a error return type likely eliminates any RVO and again plays a
> > > huge diminishing role in using functional programming methods.  The
two
> > > approaches for output variables become:
> > >     1) Fill-in out parameter reference.  [anti-FP]
> > >     2) Return a tuple<error, real_return>.
> > >
> >
> > The approach we use in this case is called StatusOr<T>; it wraps the
type
> > being returned and the associated Status object (what we call the error
> > object). This is actually a really common pattern outside of C++ and
> > parallels the error monad type found in a lot of functional programming
> > languages (we do not actually support the fmap operation, but the usage
> > pattern is semantically equivalent).
>
> StatusOr requires everything you are returning to have a [public] default
> constructor available to StatusOr and copy operations.
>
> >
> >
> > > >         - Exceptions are not typed
> > >
> > > Exceptions are strongly typed in the exact same type system as any
> > > reference.  I don't know what this statement means.
> >
> >
> > Sorry, what I meant by this is exceptions are unchecked in C++ so you
can
> > throw any type; hence, there is no way to enforce what kinds of
exceptions
> > are thrown and that those exceptions are caught in functions that do not
> > forward them as in Java.
>
> So we are capable of writing a guideline that we should not use globals
> but we are incapable of writing a guidelines that requires all
> exceptions being derived from std::exception?  Oh, wait... E.14 already
> implies this.

Snarkiness aside. If the exception E thrown in function foo() is derived
from
std::exception.  How does this help enforce that bar() who calls foo()
either
catches or rethrows E?

My experience with exceptions is that make code difficult to read, that
could
just be a personal preference thing.  With an enumeration of status codes
defined for a function, the calling function knows all the possibly return
values.
With exceptions, I never know what exceptions might be thrown by code a few
layers deep.  And if a submodule defines a new exception, there's not good
way
to ensure upper level modules catch that exception.  It may hit a default
handler
but it won't be properly handled.

Why are you concerned with ROV?   It sounds like a premature optimization.
If
 the use of a status code or object, enhances the
readability/maintainability of
the code, I think it's worthwhile to consider.

>
> > Re: Virtual Functions
> > ----------------------------
>
> > Interfaces, and thus virtual functions are required for good unit
> > testing and mocking,
>
> I agree with the statement "Interfaces are required for good unit
> testing".  I do not agree with the "and thus virtual functions".  You
> can also solve this at link time with two alternative implementations of
> the class.
>
> I'm frankly amazed G-Mock doesn't allow this.  The only rationale I can
> come up with is that "big" processors tend to have branch predictors on
> virtual function tables while "small" processors do not, so for the
> environments Google typically targets there really isn't any thought
> put into the cost of virtual.
>
> Though, marking everything virtual also eliminates any sort of 'inline',
> so I am somewhat perplexed even on that aspect as the basis.
>
>
> Back to Big Picture
> ----------------------------
>
> I do feel we are seriously missing a broader philosophical question.  In
> what ways do you feel the CppCoreGuidelines do not address all of your
> coding guidelines suggestions?  Rather than write an entirely new
> document why would we not simply point to that and then extend it if we
> feel it is insufficient in a few key areas.  As you saw in my previous
> email, nearly everything we agree on comes from the CCG already and
> nearly everything we disagree on is a contradiction from the CCG.

Our intention isn't to come up with a new document.  In our summit
meetings, you agreed we could propose a coding guide as you didn't
have anything spelled out and you didn't want to tackle it.  So we put
together an initial proposal and we're discussing it.  If your decision is
to say that CCG is the final say for all coding practices in openBMC
then we'll have to figure out what that means for us.

> I do not want to hear that the CCG is "too long".  If you take out the
> references it is in the same order of magnitude as the GSG.  It is also
> organized in a way where each section is a straight-forward statement to
> be followed, so anyone unfamiliar or uninterested can simply read the
> TOC to understand all of the rules (ex. "I.2 Avoid global variables.",
> E.6 Use RAII to prevent leaks").

"You do not want to hear"...I don't understand why you state is in such
a manner or assume we'd make this point.

> [1] http://wiki.qt.io/Qt_Coding_Style
> [2] http://wiki.qt.io/Coding_Conventions
> [3] https://github.com/open-power/hostboot
> --
> Patrick Williams


----------
Nancy

[-- Attachment #2: Type: text/html, Size: 27407 bytes --]

Re: C++ Feature Whitelist/Blacklist

[Nancy Yuen]

[-- Attachment #1: Type: text/plain, Size: 3346 bytes --]

My comments inline.

On Sun, Nov 6, 2016 at 7:44 PM, Patrick Williams <patrick@stwcx.xyz> wrote:
>
> Nancy,
>
> A few minor responses below.
>
> On Fri, Nov 04, 2016 at 02:46:42AM -0700, Nancy Yuen wrote:
> > 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.
>
> What I meant by this was not that every contributor is expected to know
> all of the code, but that the maintainer is likely to know the internal
> APIs of it enough to be able to review contributions.  One of the main
> points of contention is a proposal that "makes it easier to review" and
> that is the part that makes more sense when you are talking about 1+
> MLOC of interdependent code.  That particular point is less interesting
> when you are talking about <50kLOC.
>
> > Btw, Google's codebase is not monolithic.
>
> I realize that was not likely the case, but those were the words Titus
> used in his public talk.
>
> > 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"
>
> I feel you have taken this one sentence very much out of context.

I disagree.

>
> The first paragraph of the abstract says:
>
>     The C++ Core Guidelines are a collaborative effort led by Bjarne
>     Stroustrup, much like the C++ language itself. They are the result of
>     many person-years of discussion and design across a number of
>     organizations. Their design encourages general applicability and broad
>     adoption but they can be freely copied and modified to meet your
>     organization's needs.
>
> They are actively encouraging others to use this list.  The also state

I did not suggest they were encouraging people not to use their guidelines.

> in the abstract:
>
>     The rules are designed to be supported by an analysis tool. Violations
>     of rules will be flagged with references (or links) to the relevant
>     rule. We do not expect you to memorize all the rules before trying to
>     write code.
>
> In fact, clang-tidy has a number of checks for these rules to encourage
> their use.

I would love it if we could install clang-tidy to enforce these rules on
openBMC's gerrit.

>
> The one sentence you quoted I read as "We are willing to change these
> rules if evidence points to the contrary.  Please prove us wrong."  And,
> in fact, the guidelines are "actively maintained" on Github and have
> changed since their original introduction.

Actually, the sentence I quoted specifically mentions both verifying and
disproving their rules, not just disproving.  To me that says the rules
aren't perfect, may be wrong, have unintended consequences, scenarios when
it doesn't work etc.  To me it says please feel free to check that rules
have the intended affect and not use them blindly.

>
> --
> Patrick Williams

----------
Nancy

[-- Attachment #2: Type: text/html, Size: 4210 bytes --]

Re: C++ Feature Whitelist/Blacklist

[Patrick Williams]

[-- Attachment #1: Type: text/plain, Size: 4121 bytes --]

On Mon, Nov 07, 2016 at 02:25:30AM -0800, Nancy Yuen wrote:
> > Back to Big Picture
> > ----------------------------
> >
> > I do feel we are seriously missing a broader philosophical question.  In
> > what ways do you feel the CppCoreGuidelines do not address all of your
> > coding guidelines suggestions?  Rather than write an entirely new
> > document why would we not simply point to that and then extend it if we
> > feel it is insufficient in a few key areas.  As you saw in my previous
> > email, nearly everything we agree on comes from the CCG already and
> > nearly everything we disagree on is a contradiction from the CCG.
> 
> Our intention isn't to come up with a new document.  In our summit
> meetings, you agreed we could propose a coding guide as you didn't
> have anything spelled out and you didn't want to tackle it.  So we put
> together an initial proposal and we're discussing it.  If your decision is
> to say that CCG is the final say for all coding practices in openBMC
> then we'll have to figure out what that means for us.
> 
> > I do not want to hear that the CCG is "too long".  If you take out the
> > references it is in the same order of magnitude as the GSG.  It is also
> > organized in a way where each section is a straight-forward statement to
> > be followed, so anyone unfamiliar or uninterested can simply read the
> > TOC to understand all of the rules (ex. "I.2 Avoid global variables.",
> > E.6 Use RAII to prevent leaks").
> 
> "You do not want to hear"...I don't understand why you state is in such
> a manner or assume we'd make this point.
> 
> > --
> > Patrick Williams
> 
> ----------
> Nancy

Nancy,

From my perspective, I've pointed to the CCG as the primary document
from the very beginning.  At the summit I recall hearing two arguments
against it (and it could very well be my misremembering):

    1.  It is insufficient.
    2.  It is too long.  (I remember specifically 'reams of paper' being
        thrown about).

It is really hard for me to refute your team's perspective of it being
insufficient without examples.  You did point out the 'iostreams'
direction in our discussions and I also freely admit both CCG and our
current 'guidelines' are lacking in what is classically called 'style'
(indentation, naming conventions, etc.).

I was also unable to on the spot refute the "it is too long".  It is
easy to assume that the Google Style Guide is shorter, but after this
discussion began I checked the two to compare and came to the conclusion
they are in the same ballpark.

I assumed, perhaps incorrectly, that your team in taking up the "style
guide" would focus on #1 (where you feel it is insufficient).  It is
very frustrating to see the proposal simply be a rephrasing of the most
contentious points of the GSG, because it contradicts the CCG in these
areas.

I would like to call a "reset" to this discussion.  It is much easier to
critique a formal proposal than a supposed proposal.  I have therefore
spent this morning writing my initial take at a style guide.  I have
attempted to incorporate all of the areas your team (and a few non-vocal
members of our team) have expressed where the current guidelines are
incomplete, without violating or changing any of the current guidelines.
I do still treat the CCG as the "gold standard" here and have very few
exceptions to it recorded.  I have kept this proposal exclusively to
items that are not covered by the CCG.

Please have your team review this proposal for any areas that you feel
it is lacking in specificity.

    https://gerrit.openbmc-project.xyz/1024

These guidelines are also not "set in stone" and I am willing to deviate
from the CCG if we have good rationale and can come to a consensus as a
project.  I would like to see us first "approve roughly what is the
current mode of operations" and then subsequently "propose any
deviations".  I think each deviation should be approved on its own
merits and not as a hindrance on the document as a whole.

Is this acceptable?

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: C++ Feature Whitelist/Blacklist

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 718 bytes --]

On Mon, 2016-11-07 at 02:26 -0800, Nancy Yuen wrote:
> >     The rules are designed to be supported by an analysis tool. Violations
> >     of rules will be flagged with references (or links) to the relevant
> >     rule. We do not expect you to memorize all the rules before trying to
> >     write code.
> >
> > In fact, clang-tidy has a number of checks for these rules to encourage
> > their use.
> 
> I would love it if we could install clang-tidy to enforce these rules on openBMC's gerrit.

+1 for supplementing whatever document we create with tools. They can
get us a long of the way towards consistency, and any manually tweaked
inconsistencies can be discussed in code review.

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: C++ Feature Whitelist/Blacklist

[Nancy Yuen]

[-- Attachment #1: Type: text/plain, Size: 4713 bytes --]

----------
Nancy

On Mon, Nov 7, 2016 at 11:57 AM, Patrick Williams <patrick@stwcx.xyz> wrote:
>
> On Mon, Nov 07, 2016 at 02:25:30AM -0800, Nancy Yuen wrote:
> > > Back to Big Picture
> > > ----------------------------
> > >
> > > I do feel we are seriously missing a broader philosophical question.
In
> > > what ways do you feel the CppCoreGuidelines do not address all of your
> > > coding guidelines suggestions?  Rather than write an entirely new
> > > document why would we not simply point to that and then extend it if
we
> > > feel it is insufficient in a few key areas.  As you saw in my previous
> > > email, nearly everything we agree on comes from the CCG already and
> > > nearly everything we disagree on is a contradiction from the CCG.
> >
> > Our intention isn't to come up with a new document.  In our summit
> > meetings, you agreed we could propose a coding guide as you didn't
> > have anything spelled out and you didn't want to tackle it.  So we put
> > together an initial proposal and we're discussing it.  If your decision
is
> > to say that CCG is the final say for all coding practices in openBMC
> > then we'll have to figure out what that means for us.
> >
> > > I do not want to hear that the CCG is "too long".  If you take out the
> > > references it is in the same order of magnitude as the GSG.  It is
also
> > > organized in a way where each section is a straight-forward statement
to
> > > be followed, so anyone unfamiliar or uninterested can simply read the
> > > TOC to understand all of the rules (ex. "I.2 Avoid global variables.",
> > > E.6 Use RAII to prevent leaks").
> >
> > "You do not want to hear"...I don't understand why you state is in such
> > a manner or assume we'd make this point.
> >
> > > --
> > > Patrick Williams
> >
> > ----------
> > Nancy
>
> Nancy,
>
> From my perspective, I've pointed to the CCG as the primary document
> from the very beginning.  At the summit I recall hearing two arguments
> against it (and it could very well be my misremembering):
>
>     1.  It is insufficient.
>     2.  It is too long.  (I remember specifically 'reams of paper' being
>         thrown about).
>
> It is really hard for me to refute your team's perspective of it being
> insufficient without examples.  You did point out the 'iostreams'
> direction in our discussions and I also freely admit both CCG and our
> current 'guidelines' are lacking in what is classically called 'style'
> (indentation, naming conventions, etc.).
>
> I was also unable to on the spot refute the "it is too long".  It is
> easy to assume that the Google Style Guide is shorter, but after this
> discussion began I checked the two to compare and came to the conclusion
> they are in the same ballpark.
>
> I assumed, perhaps incorrectly, that your team in taking up the "style
> guide" would focus on #1 (where you feel it is insufficient).  It is
> very frustrating to see the proposal simply be a rephrasing of the most
> contentious points of the GSG, because it contradicts the CCG in these
> areas.
>
> I would like to call a "reset" to this discussion.  It is much easier to
> critique a formal proposal than a supposed proposal.  I have therefore
> spent this morning writing my initial take at a style guide.  I have
> attempted to incorporate all of the areas your team (and a few non-vocal
> members of our team) have expressed where the current guidelines are
> incomplete, without violating or changing any of the current guidelines.
> I do still treat the CCG as the "gold standard" here and have very few
> exceptions to it recorded.  I have kept this proposal exclusively to
> items that are not covered by the CCG.
>
> Please have your team review this proposal for any areas that you feel
> it is lacking in specificity.
>
>     https://gerrit.openbmc-project.xyz/1024
>
> These guidelines are also not "set in stone" and I am willing to deviate
> from the CCG if we have good rationale and can come to a consensus as a
> project.  I would like to see us first "approve roughly what is the
> current mode of operations" and then subsequently "propose any
> deviations".  I think each deviation should be approved on its own
> merits and not as a hindrance on the document as a whole.
>
> Is this acceptable?
>

Thank you for putting this together.  Having this openBMC programming
guideline will be really helpful for my team, especially for new members.
I will follow up with my team later today to see if it meets their needs.

With respect to this proposal, and any other issues in general that arise,
as long as it meets our needs and doesn't prevent us from making forward
progress with our servers, we will try to work with it.


> --
> Patrick Williams

[-- Attachment #2: Type: text/html, Size: 5833 bytes --]