<div dir="ltr"><div class="gmail_extra"><br>On Thu, Nov 3, 2016 at 5:03 AM, Patrick Williams <<a href="mailto:patrick@stwcx.xyz">patrick@stwcx.xyz</a>> wrote:<br>><br>> On Wed, Nov 02, 2016 at 05:02:27PM -0700, Brendan Higgins wrote:<br>> >     * it seems that Patrick and I have general agreement on (just not<br>> > necessarily exact<br>> >       strength of rule):<br>> >         - Variables of class type with static storage duration (including<br>> > constants),<br>> >           plain old data types (POD) are allowed<br>><br>> I'm not convinced we have agreement on this one.<br>><br>> > > I would<br>> > > prefer we start with industry recognized experts opinion[3] and then<br>> > > document deviations specific to OpenBMC as they come up.<br>> ><br>> ><br>> > I agree with trying to follow generally accepted practices and it seems<br>> > like the CppCoreGuidelines seem to encode that pretty well, but it is not<br>> > really the same as a style guide and I see no reason why that precludes us<br>> > from writing one.<br>><br>> There is obviously a major cultural difference here (between Google and<br>> "everyone else").  What Google calls a style guide is a combination of<br>> what others call two different documents: a coding style guide and a<br>> coding conventions (or guidelines) document.  Style guides describe the<br>> location of spaces, names of variables, etc.  Coding guidelines give you<br>> the "Do's and Don'ts".  The QT project, for example, has called them<br>> exactly that: "coding style"[1] and "coding conventions"[2].<br>><br>> I honestly think that the difference between the two is partially to<br>> blame for why we have been talking past each other previously in the<br>> need for a "style guide".<br>><br><br>Agreed, and I thought we discussed this in our meetings last week.  What we'd<br>like to define is more than just style.  I don't care what we call it though.<br>A style guide, language guidelines, rules.  But having these conventions and<br>rules around helps programmers contribute, especially programmers who are just<br>getting started with OpenBMC.<br><br>> > I would, however, caution against citing the opinions of<br>> > "industry recognized experts" as sole justification for something. If the<br>> > opinion itself is cited with good reasoning/good supported evidence that's<br>> > fine, but it is important not to discount someone because he disagrees with<br>> > someone important. I am not saying you did that anywhere, all of the<br>> > arguments you made below either make clear arguments that stand on their<br>> > own or cite well thought out self-contained arguments, just something to<br>> > keep in mind.<br>><br>> Also be aware that this is not some weak "appeal to authority" here.<br>> We're talking about Bjorn and Herb.  In case you are not familiar, they<br>> are the creator of C++ and the convener of the standards body<br>> respectively.  Discounting their opinion is like saying Linus doesn't<br>> know what he is talking about.  The balance is surely tipped in their<br>> favor coming into any discussion and you better have some pretty<br>> significant argument against it.<br>><br><br>I'm not sure why sarcasm is necessary here.  It's not conducive to a productive<br>discussion.<br><br>Yes, we're all familiar with Bjorn and Herb and their roles in the standards and<br>the C++ language.  Doesn't mean they're never wrong.  There's no discounting of<br>their opinion happening.  Just simply asking for good supported reasoning.  You<br>asked for the same when we brought up virtual functions.  Even in the<br>CppCoreGuidelines abstract, it states these rules may be too strict, may need<br>changing or more exceptions.  It even asks for feedback:  "Please try to verify<br>or disprove rules! In particular, we'd really like to have some of our rules<br>backed up with measurements or better examples."  It's a work in progress.<br>I don't know about you, but I've been taught never to just follow rules<br>unquestioningly, but to ask questions.<br><br>So far, it sounds like we mostly agree on the following:<br><br>> > Re: Implicit conversion constructors and operators<br>> > Re: Multiple Inheritance<br><br>I don't think it hurts to call it out though.  Bear in mind, this guide should also<br>help for future developers contributing to OpenBMC, and their experience levels<br>may vary.<br><br>> > Re: Variables of class type with static storage duration (including<br>> > constants)<br>> > -----------------------------------------------------------------------------------------------------<br>> ><br>> > This proposal is a key example on why it is hard to quantify these types<br>> > > of rules.  Everyone agrees with a general principle of "reducing<br>> > > globals", but as spelled out this proposal would prohibit<br>> > > initializer-list construction, std::string, and the pattern within our<br>> > > IPMI plugins of "register on load".  These are all reasonable exceptions<br>> > > that require little justification in my mind.<br>> ><br>> ><br>> > It sounds like we agree in principle. My proposal would prohibit<br>> > std::string constants, but it would not prevent their general use.<br>> > Initializer list construction would not be prohibited for c-style arrays<br>> > and not its use in general; you would not be able to use it for<br>> > constructing variables of class type with static storage, but it outright<br>> > prohibits that anyway. As written currently, yes the ipmid plugin<br>> > registration code would not conform, but it would be possible to write it<br>> > in a way that does conform; this is actually something I am working on with<br>> > my changes to ipmid, but that is a discussion for elsewhere. (I would like<br>> > to be clear that this does not discourage global pointers, but in general<br>> > these also would require a good justification).<br>><br>> The only time there is unspecified behavior in the standard is when you<br>> are using a symbol defined in another compilation unit in your own<br>> compilation unit during the initialization phase of your object.<br>><br><br>Are you saying this won't happen or happens infrequently?  Or that<div>developers won't accidentally write code that triggers this?<div><br>> I do not understand why you would want to prohibit std::string constants<br>> and any other initializer-list constructed data structures when they are<br>> not the issue.  We are often generating a data structure from machine<br>> description files and using the initializer-list syntax to prime those<br>> structures.  Having to do it outside an initializer-list requires a<br>> special "globalDataInit()" function which is a needless and less<br>> efficient hoop to jump through.<br><br>Initialize-list constructed data structures are not prohibited in general,<br>only for class types with static storage.<br><br>><br>> In my view, I.22 already has everything covered.  It also has a better<br>> description of the problem areas in the "Enforcement" heading<br>> (non-constexpr functions, extern objects).<br>><br>> ><br>> > Re: Implicit conversion constructors and operators<br>> > -------------------------------------------------------------------<br>> ><br>> > This would prohibit 'unique_ptr<T>::operator bool()' and<br>> > > 'string::string(const char*)', which are part of the STL and exist<br>> > > for concise syntax.  I do agree with this in most cases but there are<br>> > > cases where they both obvious and crisper.  A non-obvious conversion<br>> > > should be prohibited.<br>> ><br>> ><br>> > Maybe I should have called out more clearly that I do not plan on<br>> > prohibiting widely used parts of the styleguide that do these types of<br>> > things. In anycase, the smart pointers are special in that they maintain<br>> > the API that exists for raw pointers and it is common practice for people<br>> > to check for a nullptr by simply treating it as a boolean value.<br>> > Nevertheless, the vast majority of custom APIs should not use this feature,<br>> > and you seem to agree with this point in general anyway.<br>><br>> I think I agree, it just feels odd to point out.  Who writes an operator<br>> overload "just because"?  Everyone who writes one thinks that the<br>> rationale for it is obvious.<br>><br>> > Re: Multiple Inheritance<br>> > --------------------------------<br>> ><br>> > > Composition is fairly common practice in C++ which uses multiple<br>> > > inheritance.  The sdbusplus bindings, as an example, have a specific<br>> > > template to aid in the composition of N dbus interfaces into a single<br>> > > C++ class. [4]<br>> ><br>> ><br>> > Your argument seems to be arguing for allowing multiple inheritance *of<br>> > interfaces*, which I meant to allow. I said "unless at most one of the<br>> > parent classes is an interface i.e. only has pure virtual methods", it<br>> > should have read "unless at most one of the parent classes is *not* an<br>> > interface i.e. only has pure virtual methods". You can inherit from as many<br>> > interfaces as you want, but only one (or no) non-interface. Technically,<br>> > your sdbusplus bindings are not interfaces, but I could see an exception<br>> > for that, because they are fairly special constructs. *It quacks like an<br>> > interface*, from the standpoint of an implementor. Also, the CCG sections<br>> > you cited agree with this standpoint. C.135 specifically calls out<br>> > interfaces and C.136 seemed to want to discourage the behaviour in the<br>> > reasoning section.<br>><br>> Right.  There is an obvious overload of the word "interface" in this<br>> case.  Someone reviewing what you wrote is coming at it with the Java<br>> language "interface" mindset: a pure-virtual class.  The DBus Interface<br>> bindings are clearly not pure-virtual but they are "interface-like".<br>><br>> Again, multiple inheritance isn't something that I've seen people even<br>> attempt typically unless they know what they are doing.  So it seems odd<br>> to point it out but then exempt "obvious" usage.<br>><br>> > Re: Non-const-non-r-value references<br>> > ---------------------------------------------------<br>> ><br>> > > I find this especially unnecessary, not recommended anywhere outside of<br>> > > the GSG, and having a crippling effect to the language.  C++ is not a<br>> > > procedural / OO programming language; it is also a functional<br>> > > programming language.  The bulk of the STL is to facilitate a<br>> > > functional programming style and many of the C++11 features (lambdas,<br>> > > constexpr functions, etc.) take large inspiration from FP.  Eliminating<br>> > > l-value reference makes FP techniques exceptionally harder and<br>> > > syntactically clumsier.<br>> ><br>> ><br>> > Again, I explicitly allowed use where necessary to maintain consistency our<br>> > utilize the STL.<br>><br>> The logical conclusion of this exemption is to actively encourage use of<br>> non-const l-value references, which is surely not what you intend<br>> (though, I do).<br>><br>>     - As an author of an API, I do not know how users are going to<br>>       organize their own data.<br>>     - As an author of an API, I should do nothing to preclude reasonable<br>>       programming patterns.<br>>     - All container iterators work by default on l-value references.<br>>     - Functors and lambdas to the STL algorithms work easiest on l-value<br>>       references.<br>>     - Therefore, I should conclude that my users could have a collection<br>>       of things and could desire to use functional programming style,<br>>       and I should make life easier for them by taking an l-value<br>>       reference.<br>><br>> > But in general, const references, r-value references, and<br>> > pointers together fulfill the same purposes and make it easier to<br>> > understand what the code is doing. Const reference implies nothing will<br>> > happen to the data, it will not (outside of exceptional circumstances)<br>> > store that reference and will not modify it; basically it stays in the<br>> > function. No special semantics are required when calling the function<br>> > because there is nothing to consider. R-value reference implies that the<br>> > data will be consumed, you do not own it anymore and it does it in a<br>> > beautifully explicit way, std::move, which means that you know the data<br>> > does not belong to you anymore when you call the function. Providing a<br>> > pointer requires you to either be working with a pointer which is clear<br>> > because it has different semantics, or you have to pass a pointer<br>> > explicitly; either way, you know that the function is free to edit what you<br>> > passed in; it should not store it because it does not own it (otherwise use<br>> > a smart pointer), but is free to edit it in the context of the function<br>> > call.<br>><br>> I understand what you and the GSG are saying.  I just disagree with it.<br>><br>> In C, everything is a pointer.  I could just write all of my code to not<br>> use references at all and instead use 'T*' and 'const T*'.  You now<br>> still have no idea when you read 'foo(&bar, &baz)'.<br>><br>> In C++ there is always const_cast, so even if something is passed by<br>> const it _could_ be manipulated.  (Not to say this is good practice in<br>> any way.  See ES.50).<br>><br>> It is a matter of public record[3] that I have reviewed on roughly<br>> 50-100kLOC a year for the last 5 years and I still don't buy into this<br>> reviewer argument.  If someone is calling an API and I don't know what<br>> that API is doing, I don't give them a +1 no matter how "obvious" it may<br>> seem.<br>><br>> The rough summary of my viewpoint is:<br>>     1) This syntax simply lulls you into a false sense of security as<br>>        the reviewer.<br>>     2) This syntax greatly diminishes aspects of modern C++ practices.<br>>     3) This syntax is not recommended anywhere outside of Google.<br>><br>> > I do not see it standing in the way of FP at all, in FP you are<br>> > usually dealing with immutable data types which you should use as much as<br>> > possible i.e. const references, in the case that is expensive you have<br>> > r-value references and pointers.<br>><br>> FP style does not always mean immutable.  A toy example:<br>><br>>     vector<int> stuff = ...;<br>>     auto times2 = [](auto& x) { x *= 2; }<br>>     for_each(stuff, times2);<br>><br>> > Although, the CCG does say it is allowed,<br>> > it admits that non-const references can lead to bugs see the note under<br>> > F.17.<br>><br>> It is a pretty weak argument for you to use a sectioned titled "For<br>> 'in-out' parameters, pass by reference to non-const" as evidence.<br>><br>> The "error" they are talking about is a reassertion of F.16: "For 'in'<br>> parameters, pass ... by reference to const".  Non-const implies it could<br>> be changed.<br>><br>> ><br>> > A very common recommendation in "Exceptional Modern C++" is to define a<br>> > > function like:<br>> > >     template <typename T> foo(T&&)<br>> > > This function operates on both l-value and r-value references<br>> > > ("universal references" as Meyers calls them).<br>> ><br>> ><br>> > I think I am missing something here. l-value and r-value references are two<br>> > different things and are treated differently by the compiler. I tried the<br>> > following:<br>> ><br>> > int add(int&& l, int&& r) {<br>> >   return l + r;<br>> > }<br>> ><br>> > int main() {<br>> >   int l = 1;<br>> >   int r = 2;<br>> >   std::cout << "add(1, 2) = " << add(l, r) << std::endl;<br>> >   return 0;<br>> > }<br>> ><br>> > and got the following compile error:<br>> ><br>> > test.cc:10:42: error: cannot bind ‘int’ lvalue to ‘int&&’<br>> >    std::cout << "add(1, 2) = " << add(l, r) << std::endl;<br>> ><br>> > which is precisely what I expected to happen. I think I am missing part of<br>> > your example.<br>><br>> What I wrote and what you tested are two entirely different things.  I<br>> wrote a template; you tested a non-template.  Templates and autos fall<br>> under type-deduction rules.  Read up on those and reference collapsing.<br>><br>> foo(10) => foo(int&&)<br>> foo(x) => foo(int& &&) => foo(int&)<br>><br>> ><br>> > Re: Exceptions<br>> > --------------------<br>> ><br>> > > This was the "old way" in C++ and the standards body specifically<br>> > > deprecated what you are suggesting.  I'm sure you could read their<br>> > > rationale.<br>> ><br>> ><br>> > Sure, I would be happy to. I tried googling around and the only recent<br>> > update I could find relating to how the compiler treats exceptions was the<br>> > addition of noexcept. Could you share this?<br>><br>> See deprecation of 'throw(typeid, ...)'. :<br>> <a href="http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3051.html">http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3051.html</a><br>><br>> (I haven't actually read the reasoning myself, I just know it is how it<br>> was previously done and is now deprecated.)<br>><br>> ><br>> > > In GCC, at least, function attributes can only be defined as part of<br>> > > declaration and not definition.  That means you would have to forward<br>> > > declare every function that returns your error type.<br>> ><br>> ><br>> > That's true. Clang allows you to put the attribute on the type definition,<br>> > I forgot gcc does not allow that. Still, if the function was part of a<br>> > public interface, you would have to forward declare anyway. In anycase, my<br>> > point was that there are alternatives to exceptions where the compiler will<br>> > actually bug you if you ignore it.<br>> ><br>> > > In every large project I have been involved with, an integer quickly<br>> > > becomes insufficient to express "an error".  The projects all make their<br>> > > own "error object".  According to Titus' talk, Google has one of these<br>> > > internally as well.  So that means we're now talking about passing<br>> > > around a heavy object by value or a pointer to one (which is always at<br>> > > risk of leaking)?  What does this error look like?<br>> ><br>> ><br>> > Also true, hypothetically we would use some sort of error type. Probably an<br>> > error enum and an optional string message. Google has an open source one we<br>> > could use, or we could write our own. In any case, it does not need to be<br>> > heavy weight, as long as the field list is small the object will be small.<br>> > For user defined types that fit inside of a few machine types, gcc and<br>> > clang will frequently pass them via register instead of stack. In any case,<br>> > I would expect the error object to be returned so it would be subject to<br>> > return value optimization.<br>> ><br>> > > Having a error return type likely eliminates any RVO and again plays a<br>> > > huge diminishing role in using functional programming methods.  The two<br>> > > approaches for output variables become:<br>> > >     1) Fill-in out parameter reference.  [anti-FP]<br>> > >     2) Return a tuple<error, real_return>.<br>> > ><br>> ><br>> > The approach we use in this case is called StatusOr<T>; it wraps the type<br>> > being returned and the associated Status object (what we call the error<br>> > object). This is actually a really common pattern outside of C++ and<br>> > parallels the error monad type found in a lot of functional programming<br>> > languages (we do not actually support the fmap operation, but the usage<br>> > pattern is semantically equivalent).<br>><br>> StatusOr requires everything you are returning to have a [public] default<br>> constructor available to StatusOr and copy operations.<br>><br>> ><br>> ><br>> > > >         - Exceptions are not typed<br>> > ><br>> > > Exceptions are strongly typed in the exact same type system as any<br>> > > reference.  I don't know what this statement means.<br>> ><br>> ><br>> > Sorry, what I meant by this is exceptions are unchecked in C++ so you can<br>> > throw any type; hence, there is no way to enforce what kinds of exceptions<br>> > are thrown and that those exceptions are caught in functions that do not<br>> > forward them as in Java.<br>><br>> So we are capable of writing a guideline that we should not use globals<br>> but we are incapable of writing a guidelines that requires all<br>> exceptions being derived from std::exception?  Oh, wait... E.14 already<br>> implies this.<div><br></div><div>Snarkiness aside. If the exception E thrown in function foo() is derived from</div><div>std::exception.  How does this help enforce that bar() who calls foo() either</div><div>catches or rethrows E?</div><div><br></div><div>My experience with exceptions is that make code difficult to read, that could</div><div>just be a personal preference thing.  With an enumeration of status codes</div><div>defined for a function, the calling function knows all the possibly return values.  </div><div>With exceptions, I never know what exceptions might be thrown by code a few</div><div>layers deep.  And if a submodule defines a new exception, there's not good way</div><div>to ensure upper level modules catch that exception.  It may hit a default handler</div><div>but it won't be properly handled.</div><div><br></div><div>Why are you concerned with ROV?   It sounds like a premature optimization.  If</div><div> the use of a status code or object, enhances the readability/maintainability of</div><div>the code, I think it's worthwhile to consider. </div><div><br></div><div><div>><br></div><div>> > Re: Virtual Functions<br>> > ----------------------------<br>><br>> > Interfaces, and thus virtual functions are required for good unit<br>> > testing and mocking,<br>><br>> I agree with the statement "Interfaces are required for good unit<br>> testing".  I do not agree with the "and thus virtual functions".  You<br>> can also solve this at link time with two alternative implementations of<br>> the class.<br>><br>> I'm frankly amazed G-Mock doesn't allow this.  The only rationale I can<br>> come up with is that "big" processors tend to have branch predictors on<br>> virtual function tables while "small" processors do not, so for the<br>> environments Google typically targets there really isn't any thought<br>> put into the cost of virtual.<br>><br>> Though, marking everything virtual also eliminates any sort of 'inline',<br>> so I am somewhat perplexed even on that aspect as the basis.<br>><br>><br>> Back to Big Picture<br>> ----------------------------<br>><br>> I do feel we are seriously missing a broader philosophical question.  In<br>> what ways do you feel the CppCoreGuidelines do not address all of your<br>> coding guidelines suggestions?  Rather than write an entirely new<br>> document why would we not simply point to that and then extend it if we<br>> feel it is insufficient in a few key areas.  As you saw in my previous<br>> email, nearly everything we agree on comes from the CCG already and<br>> nearly everything we disagree on is a contradiction from the CCG.</div><div><br></div><div>Our intention isn't to come up with a new document.  In our summit</div><div>meetings, you agreed we could propose a coding guide as you didn't</div><div>have anything spelled out and you didn't want to tackle it.  So we put</div><div>together an initial proposal and we're discussing it.  If your decision is</div><div>to say that CCG is the final say for all coding practices in openBMC</div><div>then we'll have to figure out what that means for us.</div><div><br></div><div>> I do not want to hear that the CCG is "too long".  If you take out the<br>> references it is in the same order of magnitude as the GSG.  It is also<br>> organized in a way where each section is a straight-forward statement to<br>> be followed, so anyone unfamiliar or uninterested can simply read the<br>> TOC to understand all of the rules (ex. "I.2 Avoid global variables.",<br>> E.6 Use RAII to prevent leaks").<br><br>"You do not want to hear"...I don't understand why you state is in such </div><div>a manner or assume we'd make this point.</div><div><br></div><div>> [1] <a href="http://wiki.qt.io/Qt_Coding_Style">http://wiki.qt.io/Qt_Coding_Style</a><br>> [2] <a href="http://wiki.qt.io/Coding_Conventions">http://wiki.qt.io/Coding_Conventions</a><br>> [3] <a href="https://github.com/open-power/hostboot">https://github.com/open-power/hostboot</a><br>> --<br>> Patrick Williams<br><br></div></div></div></div><div><br>----------<br>Nancy</div></div></div>