Google Code Style Comments

gistfile1.txt

https://google.github.io/styleguide/cppguide.html#C++_Version

- We're using C++11, nothing newer

https://google.github.io/styleguide/cppguide.html#Header_Files
https://google.github.io/styleguide/cppguide.html#Self_contained_Headers
https://google.github.io/styleguide/cppguide.html#The__define_Guard
https://google.github.io/styleguide/cppguide.html#Inline_Functions
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions
https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors
https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types
https://google.github.io/styleguide/cppguide.html#Inheritance
https://google.github.io/styleguide/cppguide.html#Write_Short_Functions
https://google.github.io/styleguide/cppguide.html#Function_Overloading
https://google.github.io/styleguide/cppguide.html#Default_Arguments
https://google.github.io/styleguide/cppguide.html#Friends
https://google.github.io/styleguide/cppguide.html#noexcept
https://google.github.io/styleguide/cppguide.html#Run-Time_Type_Information__RTTI_
https://google.github.io/styleguide/cppguide.html#Casting
https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros
https://google.github.io/styleguide/cppguide.html#0_and_nullptr/NULL
https://google.github.io/styleguide/cppguide.html#Template_metaprogramming
https://google.github.io/styleguide/cppguide.html#Nonstandard_Extensions
https://google.github.io/styleguide/cppguide.html#Aliases
https://google.github.io/styleguide/cppguide.html#General_Naming_Rules
https://google.github.io/styleguide/cppguide.html#Type_Names
https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_Grammar
https://google.github.io/styleguide/cppguide.html#Formatting
https://google.github.io/styleguide/cppguide.html#Non-ASCII_Characters
https://google.github.io/styleguide/cppguide.html#Floating_Literals
https://google.github.io/styleguide/cppguide.html#Return_Values
https://google.github.io/styleguide/cppguide.html#Horizontal_Whitespace
https://google.github.io/styleguide/cppguide.html#Exceptions_to_the_Rules
https://google.github.io/styleguide/cppguide.html#Windows_Code
https://google.github.io/styleguide/cppguide.html#Parting_Words

Agreed

https://google.github.io/styleguide/cppguide.html#thread_local
https://google.github.io/styleguide/cppguide.html#Implicit_Conversions
https://google.github.io/styleguide/cppguide.html#Structs_vs._Tuples
https://google.github.io/styleguide/cppguide.html#Rvalue_references
https://google.github.io/styleguide/cppguide.html#Use_of_constexpr
https://google.github.io/styleguide/cppguide.html#std_hash
https://google.github.io/styleguide/cppguide.html#Other_Features
https://google.github.io/styleguide/cppguide.html#File_Names
https://google.github.io/styleguide/cppguide.html#Function_Names
https://google.github.io/styleguide/cppguide.html#Namespace_Names
https://google.github.io/styleguide/cppguide.html#Exceptions_to_Naming_Rules
https://google.github.io/styleguide/cppguide.html#Implementation_Comments
https://google.github.io/styleguide/cppguide.html#Function_Argument_Comments
https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace

Fair Enough

https://google.github.io/styleguide/cppguide.html#Designated_initializers

Moot - we're only using C++11 features.

https://google.github.io/styleguide/cppguide.html#Include_What_You_Use

Agreed - furthermore, header files should use the most direct path to the definition of something even if something else included also includes that path.

https://google.github.io/styleguide/cppguide.html#Forward_Declarations

Agreed - a forward declaration should only be used within the file defining the entity in order to use it within its definition:

    class Node;
    typedef ptrNode *Node;
    
    class Node {
        ...
        private:
            ptrNode next;
            ptrNode previous;
        ...
    }

https://google.github.io/styleguide/cppguide.html#Namespaces

In general - agreed. However, the implementation file may use the `using namespace` declarative for the specific stuff it is implementing.
That is:

    myHeader.h
        #ifndef MY_HEADER_H__
        #define MY_HEADER_H__
        
        namespace X {
            namespace Y {
                class myClassXY {
                    myClassXY();
                    ~myClassXY();
                    ...
                }
            }
        }
        
        #endif //MY_HEADER_H__
    
    myHeaderImpl.cppguide
        #include "myHeader.h" // b/c it's immediately located
        
        ..
        // other includes
        ..
        
        using namspace X::Y;    // only one allowed
        // all other namspaces should be explicitly used
        
        myClassXY::myClassXY() {
            ...
        }
        ...
        
        //everything else

Con:
    Occassionally this can trip the compiler (really the linker); in which case the full namespace should be used to resolve.

Pro:
    It cuts down on the specific namespace being written in its implementation, so code looks a little nicer.

Enforcement: Not strictly enforced

https://google.github.io/styleguide/cppguide.html#Internal_Linkage

Agreed - though use the `static` method or a named namespace, such as `internal` within the larger namespace being implemented in the file.

https://google.github.io/styleguide/cppguide.html#Local_Variables

Agreed with the exception of using `{}` initializations.

https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

Agreed....I think??? I'll have to read through this one in more detail to understand it.


https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes

Agreed - though I'd go further to say that a `struct` may have an optional constructor only. Structs should be able to be treated like C APIs where Classes must be treated as purely C++ APIs.


https://google.github.io/styleguide/cppguide.html#Operator_Overloading

Agreed - overloading operators should be almost never done; and when done it should have a nice big comment on why it's necessary to do it.

https://google.github.io/styleguide/cppguide.html#Access_Control

Agreed; though there's times when `protected` is good form.

https://google.github.io/styleguide/cppguide.html#Declaration_Order

Agreed with caveat that even empty sections should still be listed.

https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs

Mixed - I get what they're saying, but using parameters to pass out data from the function is always clunky, and ends up creating bad APIs.
Rather, return the output data and try to keep it to a single type. We can use exceptions so error handling isn't an issue there either.

https://google.github.io/styleguide/cppguide.html#trailing_return

Agreed; furthermore since `auto` should never be a return type for a function only the older form should be used.

https://google.github.io/styleguide/cppguide.html#Ownership_and_Smart_Pointers

Fair enough; the smart pointer classes can be hard to negotiate which is the correct one to use. However, a good ownership model needs to be defined project wide too.

https://google.github.io/styleguide/cppguide.html#Exceptions

I SO want to agree with this; however, since we are integrating to Python we're using Exceptions :(

That said, libraries need to be self-contained with exception handling (e.g an exception cannot leave the library API but must be caught and an non-exception based error mechanism used); especially when Dynamically linked libraries are used.
Also, it is *never* okay to crash the program, and errors should be handled as close to their source as possible.

https://google.github.io/styleguide/cppguide.html#Streams

Agreed; though I would go further and just say avoid streams when possible.

https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement

Agreed; however I would go further and suggest that if the code needs a Postfix form then that's a red flag that something is being done wrong or an optimization is being misapplied. Use the prefix form and refactor the code for better readability.

https://google.github.io/styleguide/cppguide.html#Use_of_const

Agreed - and we should enforce `const <type>` form.

https://google.github.io/styleguide/cppguide.html#Integer_Types

Agreed - though we should almost always use the bit-sized type. I would favor more unsigned type usage though, especially if a value should never be negative.

https://google.github.io/styleguide/cppguide.html#64-bit_Portability

Agreed - we need to be mindful of bit portability

https://google.github.io/styleguide/cppguide.html#sizeof

Disagree - there are actually whole classes of bugs related to `sizeof(varname)` as they don't return what is expected.
Often, though if you're using `sizeof` then that is generally a point that either:
- You're actually doing C instead of C++
- You're doing C++ and you're doing something wrong

https://google.github.io/styleguide/cppguide.html#Type_deduction

Agreed with caveats:

- `auto` should generally be avoided; though their example of `auto x = y.begin()` is a valid use, and the exception to the rule
- lambdas should be generally avoided;
- function return type deductions should be avoided (don't be lazy)

https://google.github.io/styleguide/cppguide.html#CTAD

Agreed; just avoid Class Template Argument Deduction entirely

https://google.github.io/styleguide/cppguide.html#Lambda_expressions
https://google.github.io/styleguide/cppguide.html#Formatting_Lambda_Expressions

?? - Lambdas should generally be avoided; and they can't be directly tested. Prefer directly testable code.
The exception is when:
- there's no choice b/c an API forces you to, in which case use a minimal lambda to call to a directly testable function
- the lambda is a one-liner type function that is extremely obvious and super simple - e.g lambda: [](button& x) -> { x.click(); }

https://google.github.io/styleguide/cppguide.html#Boost

Agreed - we should curate a list of approved Boost libraries namely b/c Boost sometimes has competing implementations (e.g boost::signal vs boost::signal2).
In general, any Boost library should be approved but we should place some constraints.

https://google.github.io/styleguide/cppguide.html#Inclusive_Language

Be nice and considerate; but don't go out of your way to define something that already has a natural, well known name.

https://google.github.io/styleguide/cppguide.html#Variable_Names

Generally Agreed - though:
- no need to have trailing underscores for anything
- it's generally useful to have a leading underscore for function parameters

https://google.github.io/styleguide/cppguide.html#Constant_Names
https://google.github.io/styleguide/cppguide.html#Enumerator_Names
https://google.github.io/styleguide/cppguide.html#Macro_Names

Mixed - I'm generally of the opinion that constants, enumerators, and C Preprocessor Macros should be all upper case names.

https://google.github.io/styleguide/cppguide.html#Comments

Agreed - furthermore: code expresses what is done; but does not carry with it the intent. Comments provide the intent. So comment things a lot.
Don't comment every line; but do describe the intent of what is happening.
If something is complex, then it probably needs to be made simpler; if not a massive comment is required to explain it and why the complexity is required.

https://google.github.io/styleguide/cppguide.html#Comment_Style

Agreed - though use `//` for short comments, and `/* */` for long block comments, or section breaks

    /*********************
     *** section break ***
     *********************/
     
    /*
     * Long Form Comments
     * That may go on for
     * many
     * many
     * lines
     */
     
     // something simple on one line
     
     // something simple too
     // even if it takes a couple
     // lines, but no more than 3 lines
     
https://google.github.io/styleguide/cppguide.html#File_Comments

Mixed - okay so historically file comments were required for VCS systems; however, nowadays VCS is a lot more capable so we don't need to track version data in the file comments.
The file comments should be minimal and related to only:
- what's necessary for licensing requirements (authors, copyright, license)
- a block describing the file if absolutely necessary (in general this shouldn't be needed; though may be more in headers than implementation).

https://google.github.io/styleguide/cppguide.html#Class_Comments
https://google.github.io/styleguide/cppguide.html#Function_Comments
https://google.github.io/styleguide/cppguide.html#Variable_Comments

Agreed - though we should be doing this in accordance with a tool like doxygen, sphinx, etc that can be used to extract and build documentation of the classes, etc. automatically for us.

https://google.github.io/styleguide/cppguide.html#Implementation_Comment_Donts

Mixed - if you're writing comments on code that is blatantly obvious than you're writing the comments wrong. Take a step back; look at the bigger picture and put the comments in the appropriate
places. If something is happening that is non-obvious then it definitely needs a comment; but otherwise it's likely that the logical block (even within a code-block) is more appriate.

https://google.github.io/styleguide/cppguide.html#TODO_Comments

Agreed - TODOs are placeholders for stuff that needs to be done soon.

https://google.github.io/styleguide/cppguide.html#Line_Length

Mixed - honestly, line length doesn't matter any more. We're not using 80-character terminals to edit code. If someone is, then they can scroll.
That said, between 100 and 150 is typically a good max; though often that can be shorted using some basic things like:
- wrapping function parameters to a function class so each is on its own line
- using multi-line strings
- breaking the functionality down to be simpler to understand (don't try to do everything at once)

https://google.github.io/styleguide/cppguide.html#Spaces_vs._Tabs

Mixed - generally I prefer tabs - editors can handle making things line up properly.
That said, for consistency especially in Git Hub PRs we should use spaces. 4 spaces per tab is better.
2 doesn't leave enough room to visually distinguish code blocks properly.

https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions
https://google.github.io/styleguide/cppguide.html#Function_Calls

Agreed; though prefer putting each on its own line

Also, with respect to function calls, place the closing brace of the function on its own line:

    x(
        param1,
        param2,
        param3,
    );
    
This helps to visually distinguish the function call from opening a new code block.

https://google.github.io/styleguide/cppguide.html#Braced_Initializer_List_Format

Mixed - generally brace initializers should be avoided. Exception may be with static/constant numeric arrays, but even then that should be extremely rare.

https://google.github.io/styleguide/cppguide.html#Conditionals
https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements

Agreed - though going further and all code blocks no matter how big/small require braces.

https://google.github.io/styleguide/cppguide.html#Pointer_and_Reference_Expressions

Agreed, though the * and & should always be with the type, not the variable.

https://google.github.io/styleguide/cppguide.html#Boolean_Expressions

Agreed, though be explicit and use braces around the individual expressions too for better clarity of intent.

https://google.github.io/styleguide/cppguide.html#Variable_and_Array_Initialization

Agreed - though keep things simple, and clear. Generally avoid brace initializations and prefer `=` initializations for basic types and `()` for classes.
Complex types can be left alone and filled in after explicitly - it's a little more code, but more readable and maintainable.

https://google.github.io/styleguide/cppguide.html#Preprocessor_Directives

Mixed - I have found it better to treat the #if blocks like code blocks, and indent them like any other code block.
I know a lot of folks like putting them at the start of a line, but that just makes the code harder to read.

    // Bad - indented directives
    if (lopsided_score) {
        #if DISASTER_PENDING  // Wrong!  The "#if" should be at beginning of line
        DropEverything();
        #endif                // Wrong!  Do not indent "#endif"
        BackToNormal();
    }
    
should be:
    
    if (lopsided_score) {
        
        #if DISASTER_PENDING  // starts a new contextual code block for the programmer
            
            DropEverything();
            
        #endif                // ends the contextual code block
        
        BackToNormal();
    }

    NOTE: The added blank lines to help with readability
    
https://google.github.io/styleguide/cppguide.html#Class_Format

Agreed; though all should always be defined.

NOTE: ordering of public/protected/private helps with ABI compatibility.

https://google.github.io/styleguide/cppguide.html#Constructor_Initializer_Lists

Agreed, though I'd keep the `:` on the first line, and then otherwise follow their examples

https://google.github.io/styleguide/cppguide.html#Namespace_Formatting

Mixed: Namespaces in headers should be indented; while in implementation files they should not.

Generally, there's only one namespace in an implementation file, and that can be declared with `using namespace`.
In headers, it's best to indent to more easily balance the declarations.

https://google.github.io/styleguide/cppguide.html#Existing_Non-conformant_Code

Fair enough; though in our case we need to be migrating code into the style we're choosing. When doing so, the whole file should be migrated in its own commit/PR.