You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Jacob Barrett <ja...@vmware.com> on 2021/05/01 00:31:52 UTC
DISCUSSION: Geode Native C++ Style and Formatting Guide
Team,
We haven’t been good about documenting our style and formatting guidance. Like the Java sources are loosely styled after the Google Java Style Guide, the C++ sources are loosely styled after the Google C++ Style Guide. We deviate in some places from the Google C++ Style Guide. Without these documented it can be confusing to new or even infrequent committers. I would like us to discuss and then document our consensus in the CONTRIBUTING.md.
If you look at https://github.com/apache/geode-native/blob/develop/CONTRIBUTING.md#style you will notice there are no deviations documented.
To my recollection we have these deviations as common practice:
https://google.github.io/styleguide/cppguide.html#C++_Version
C++ Version
Currently, code should target C++11.
[ Google: Currently, code should target C++17. ]
https://google.github.io/styleguide/cppguide.html#Forward_Declarations
Forward Declarations
Forward declarations are allowed to reduce build time. Solving for complex includes should be preferred over this. See include what you use.
[ Google: Don’t use forward declarations ]
https://google.github.io/styleguide/cppguide.html#cpplint
cpplint
[Add] Prefer using clang-tidy and clang-format with the provide configuration files to validate style and format.
https://google.github.io/styleguide/cppguide.html#Exceptions
Exceptions
We use C++ exceptions.
[ Google: We do not use C++ exceptions. ]
https://google.github.io/styleguide/cppguide.html#Boost
Boost
Use only libraries from the Boost library collection when an alternative is not available in the standard library.
[ Google: Use only approved libraries from the Boost library collection. ]
https://google.github.io/styleguide/cppguide.html#File_Names
FIle Names
File names should match the case of the class declared within. C++ files should end in .cpp and their corresponding header should end with .hpp. Header only files may use .h extension.
https://google.github.io/styleguide/cppguide.html#Variable_Names
Variable Names:
Common Variable Names:
Use camelCase variable names.
[ Google: Use _ separated variable names. ]
https://google.github.io/styleguide/cppguide.html#Function_Names
Function Names:
Regular functions have camelCase; accessors and mutators may be named like variables.
[ Google: Regular functions have mixed case; accessors and mutators may be named like variables. ]
https://google.github.io/styleguide/cppguide.html#Macro_Names
Macro Names:
Prefix all macros with ‘GEODE'
[ Google: No prefix requirement ]
https://google.github.io/styleguide/cppguide.html#File_Comments
Legal Notice and Author Line
Every file should start with the Apache 2.0 license notice.
[ Google: Every file should contain license boilerplate. Choose the appropriate boilerplate for the license used by the project (for example, Apache 2.0, BSD, LGPL, GPL).]
No author name(s) should be used in the source.
[ Google: If you make significant changes to a file with an author line, consider deleting the author line. New files should usually not contain copyright notice or author line. ]
https://google.github.io/styleguide/cppguide.html#Class_Comments
Class Comments
[Add] Use Doxygen style comment format.
https://google.github.io/styleguide/cppguide.html#Function_Comments
Function Declarations
[Add] Use Doxygen style comment format.
https://google.github.io/styleguide/cppguide.html#Variable_Comments
[Add] Use Doxygen style comment format.
https://google.github.io/styleguide/cppguide.html#TODO_Comments
TODO Comments
TODOs should include the string TODO in all caps. TODOs should not contain any personal identifying information or bug ID.
[ Google: TODOs should include the string TODO in all caps, followed by the name, e-mail address, bug ID, or other identifier of the person or issue with the best context about the problem referenced by the TODO ]
These we should adopt but haven’t out of respect of previous convention:
https://google.github.io/styleguide/cppguide.html#Enumerator_Names
Enumerator Names:
Using Googles prescribed naming convention avoids collisions with leaky macros. I am indifferent to the use of the ‘k’ prefix but since they are constants it does make it consistent. We currently have a mix of camelCase and UPPERCASE enum constants.
Proposed changes we should adopt:
https://google.github.io/styleguide/cppguide.html#Line_Length
Line Length
Each line of text in your code should be at most 100 characters long.
Pro: Consistent with Geode Java Guide.
[ Google: Each line of text in your code should be at most 80 characters long. ]
Please comment or add anything I might have missed so we can get these recorded.
Thanks,
Jake
Re: DISCUSSION: Geode Native C++ Style and Formatting Guide
Posted by Jacob Barrett <ja...@vmware.com>.
100 is just a suggestion based on the Google Java Style Guide we follow on the main repo. Why the two guides aren’t more aligned I don’t know.
> On May 3, 2021, at 2:06 PM, Robert Houghton <rh...@vmware.com> wrote:
>
> 80 characters also feels arbitrary, especially with auto-formatters (clang-tidy?) of mangling some otherwise very-readable code.
>
> From: Blake Bender <bb...@vmware.com>
> Date: Monday, May 3, 2021 at 11:23 AM
> To: dev@geode.apache.org <de...@geode.apache.org>
> Subject: RE: DISCUSSION: Geode Native C++ Style and Formatting Guide
> My $0.02 on these:
>
> Things I'd like to see us conform to Google style on:
> * I'd be happy to move to C++ 17
> * Would also be happy to remove forward declarations. "I'm not a critic, but I know what I hate," as it were, and I hate forward declarations.
> * I would also be happy with an 80-character line limit, though I don't feel strongly about it. 100 may be consistent with Geode, but it still feels arbitrary to me.
> * I would be very pleased to remove all the macros from our code. I've been bitten more than once in the past while debugging or refactoring our code, because of ill-formed macros.
>
> Google things I disagree with:
> * I don't like exceptions, but I don't even want to think about the amount of effort required to remove them from the codebase is, IMO, unreasonably high. Keep the exceptions, most of the time they're used pretty judiciously.
> * I really, really, *really* (really? Yes, really!) hate anything resembling Hungarian prefix notation, and have permanent scars from decades of reading it in Windows code. Please don't ask me to put a random 'k' in from of my enums - ick.
>
> One other note: in the past, we've had conversations about "style only" pull requests to fix some of these things, and the guidance we ended up with has been to only fix this sort of thing while you're in the code working on a fix or a feature. I, for one, would welcome some PRs that just, say, renamed a ton of member variables to replace "m_" prefix with a simple trailing "_", perhaps fixed some of the more egregious and weird abbreviations, etc. My preference for bug fixes and feature work is that all of the code changes be focused on stuff that's relevant to the fix/feature, and mixing it with random style guide refactoring, I feel, muddies the waters for future maintainers.
>
> Thanks,
>
> Blake
>
> -----Original Message-----
> From: Jacob Barrett <ja...@vmware.com>
> Sent: Saturday, May 1, 2021 9:21 AM
> To: dev@geode.apache.org
> Subject: Re: DISCUSSION: Geode Native C++ Style and Formatting Guide
>
> Great call outs!
>
>> On May 1, 2021, at 7:57 AM, Mario Salazar de Torres <ma...@est.tech> wrote:
>>
>> 1. Member variables names as of Google style guide requires a '_' char to be added at the end so it can be identified. Should we also adopt that?
>> For example, imagine you have a region mutex, so, should we name it as 'regionMutex_' ?
>
> I didn’t mention this one out in my review of differences because we are following it but I suppose with the combination of the camelCase difference we should probably call it out more specifically. Perhaps in our documentation we should show examples of both local and member variables. Do you think that will make it more clear?
>
>> 2. Also, I would like to point out that macros are dis-recommended but every C++ committee member I know.
>> What do you think about adding a notice saying: "Macros should be avoided and only used when there is no alternative”?
>
> I think that is called out in various ways in a few places in the Google guide but I am more than happy for us to include strong or clearer language around this. Between constexpr and templates there are very cases for macros anymore.
> We mostly use macros only to handle non-standard attributes. When we move to C++17 a lot of these will go away.
>
> Thanks,
> Jake
>
>
Re: DISCUSSION: Geode Native C++ Style and Formatting Guide
Posted by Jacob Barrett <ja...@vmware.com>.
Clang format and clang-tidy are enforcing many Google C++ rules and our deviations from that.
None of those tools enforce naming conventions.
I am not so much looking for this to be a holy war, nor do I think it has even remotely degraded to that. The original post was to agree on the set of currently undocumented deviations and secondarily determine if anyone has strong feelings towards changing any of those before we document it.
Do far I am not hearing any strong feelings one way or another on the deviations, nor has anyone really raised any missing deviations.
-Jake
> On May 5, 2021, at 1:24 PM, Ernie Burghardt <bu...@vmware.com> wrote:
>
> What are we missing/unable to format with our clang-tools?
> These style discussions tend to become holy wars, hopefully we can avoid this...
> if we can tool are largest concerns and perform good PR reviews looking for valid algorithms, good mnemonics naming variables and such I think we'll be doing just fine.
>
> EB
>
> On 5/3/21, 2:07 PM, "Robert Houghton" <rh...@vmware.com> wrote:
>
> 80 characters also feels arbitrary, especially with auto-formatters (clang-tidy?) of mangling some otherwise very-readable code.
>
> From: Blake Bender <bb...@vmware.com>
> Date: Monday, May 3, 2021 at 11:23 AM
> To: dev@geode.apache.org <de...@geode.apache.org>
> Subject: RE: DISCUSSION: Geode Native C++ Style and Formatting Guide
> My $0.02 on these:
>
> Things I'd like to see us conform to Google style on:
> * I'd be happy to move to C++ 17
> * Would also be happy to remove forward declarations. "I'm not a critic, but I know what I hate," as it were, and I hate forward declarations.
> * I would also be happy with an 80-character line limit, though I don't feel strongly about it. 100 may be consistent with Geode, but it still feels arbitrary to me.
> * I would be very pleased to remove all the macros from our code. I've been bitten more than once in the past while debugging or refactoring our code, because of ill-formed macros.
>
> Google things I disagree with:
> * I don't like exceptions, but I don't even want to think about the amount of effort required to remove them from the codebase is, IMO, unreasonably high. Keep the exceptions, most of the time they're used pretty judiciously.
> * I really, really, *really* (really? Yes, really!) hate anything resembling Hungarian prefix notation, and have permanent scars from decades of reading it in Windows code. Please don't ask me to put a random 'k' in from of my enums - ick.
>
> One other note: in the past, we've had conversations about "style only" pull requests to fix some of these things, and the guidance we ended up with has been to only fix this sort of thing while you're in the code working on a fix or a feature. I, for one, would welcome some PRs that just, say, renamed a ton of member variables to replace "m_" prefix with a simple trailing "_", perhaps fixed some of the more egregious and weird abbreviations, etc. My preference for bug fixes and feature work is that all of the code changes be focused on stuff that's relevant to the fix/feature, and mixing it with random style guide refactoring, I feel, muddies the waters for future maintainers.
>
> Thanks,
>
> Blake
>
> -----Original Message-----
> From: Jacob Barrett <ja...@vmware.com>
> Sent: Saturday, May 1, 2021 9:21 AM
> To: dev@geode.apache.org
> Subject: Re: DISCUSSION: Geode Native C++ Style and Formatting Guide
>
> Great call outs!
>
>> On May 1, 2021, at 7:57 AM, Mario Salazar de Torres <ma...@est.tech> wrote:
>>
>> 1. Member variables names as of Google style guide requires a '_' char to be added at the end so it can be identified. Should we also adopt that?
>> For example, imagine you have a region mutex, so, should we name it as 'regionMutex_' ?
>
> I didn’t mention this one out in my review of differences because we are following it but I suppose with the combination of the camelCase difference we should probably call it out more specifically. Perhaps in our documentation we should show examples of both local and member variables. Do you think that will make it more clear?
>
>> 2. Also, I would like to point out that macros are dis-recommended but every C++ committee member I know.
>> What do you think about adding a notice saying: "Macros should be avoided and only used when there is no alternative”?
>
> I think that is called out in various ways in a few places in the Google guide but I am more than happy for us to include strong or clearer language around this. Between constexpr and templates there are very cases for macros anymore.
> We mostly use macros only to handle non-standard attributes. When we move to C++17 a lot of these will go away.
>
> Thanks,
> Jake
>
>
>
Re: DISCUSSION: Geode Native C++ Style and Formatting Guide
Posted by Ernie Burghardt <bu...@vmware.com>.
What are we missing/unable to format with our clang-tools?
These style discussions tend to become holy wars, hopefully we can avoid this...
if we can tool are largest concerns and perform good PR reviews looking for valid algorithms, good mnemonics naming variables and such I think we'll be doing just fine.
EB
On 5/3/21, 2:07 PM, "Robert Houghton" <rh...@vmware.com> wrote:
80 characters also feels arbitrary, especially with auto-formatters (clang-tidy?) of mangling some otherwise very-readable code.
From: Blake Bender <bb...@vmware.com>
Date: Monday, May 3, 2021 at 11:23 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: RE: DISCUSSION: Geode Native C++ Style and Formatting Guide
My $0.02 on these:
Things I'd like to see us conform to Google style on:
* I'd be happy to move to C++ 17
* Would also be happy to remove forward declarations. "I'm not a critic, but I know what I hate," as it were, and I hate forward declarations.
* I would also be happy with an 80-character line limit, though I don't feel strongly about it. 100 may be consistent with Geode, but it still feels arbitrary to me.
* I would be very pleased to remove all the macros from our code. I've been bitten more than once in the past while debugging or refactoring our code, because of ill-formed macros.
Google things I disagree with:
* I don't like exceptions, but I don't even want to think about the amount of effort required to remove them from the codebase is, IMO, unreasonably high. Keep the exceptions, most of the time they're used pretty judiciously.
* I really, really, *really* (really? Yes, really!) hate anything resembling Hungarian prefix notation, and have permanent scars from decades of reading it in Windows code. Please don't ask me to put a random 'k' in from of my enums - ick.
One other note: in the past, we've had conversations about "style only" pull requests to fix some of these things, and the guidance we ended up with has been to only fix this sort of thing while you're in the code working on a fix or a feature. I, for one, would welcome some PRs that just, say, renamed a ton of member variables to replace "m_" prefix with a simple trailing "_", perhaps fixed some of the more egregious and weird abbreviations, etc. My preference for bug fixes and feature work is that all of the code changes be focused on stuff that's relevant to the fix/feature, and mixing it with random style guide refactoring, I feel, muddies the waters for future maintainers.
Thanks,
Blake
-----Original Message-----
From: Jacob Barrett <ja...@vmware.com>
Sent: Saturday, May 1, 2021 9:21 AM
To: dev@geode.apache.org
Subject: Re: DISCUSSION: Geode Native C++ Style and Formatting Guide
Great call outs!
> On May 1, 2021, at 7:57 AM, Mario Salazar de Torres <ma...@est.tech> wrote:
>
> 1. Member variables names as of Google style guide requires a '_' char to be added at the end so it can be identified. Should we also adopt that?
> For example, imagine you have a region mutex, so, should we name it as 'regionMutex_' ?
I didn’t mention this one out in my review of differences because we are following it but I suppose with the combination of the camelCase difference we should probably call it out more specifically. Perhaps in our documentation we should show examples of both local and member variables. Do you think that will make it more clear?
> 2. Also, I would like to point out that macros are dis-recommended but every C++ committee member I know.
> What do you think about adding a notice saying: "Macros should be avoided and only used when there is no alternative”?
I think that is called out in various ways in a few places in the Google guide but I am more than happy for us to include strong or clearer language around this. Between constexpr and templates there are very cases for macros anymore.
We mostly use macros only to handle non-standard attributes. When we move to C++17 a lot of these will go away.
Thanks,
Jake
Re: DISCUSSION: Geode Native C++ Style and Formatting Guide
Posted by Robert Houghton <rh...@vmware.com>.
80 characters also feels arbitrary, especially with auto-formatters (clang-tidy?) of mangling some otherwise very-readable code.
From: Blake Bender <bb...@vmware.com>
Date: Monday, May 3, 2021 at 11:23 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: RE: DISCUSSION: Geode Native C++ Style and Formatting Guide
My $0.02 on these:
Things I'd like to see us conform to Google style on:
* I'd be happy to move to C++ 17
* Would also be happy to remove forward declarations. "I'm not a critic, but I know what I hate," as it were, and I hate forward declarations.
* I would also be happy with an 80-character line limit, though I don't feel strongly about it. 100 may be consistent with Geode, but it still feels arbitrary to me.
* I would be very pleased to remove all the macros from our code. I've been bitten more than once in the past while debugging or refactoring our code, because of ill-formed macros.
Google things I disagree with:
* I don't like exceptions, but I don't even want to think about the amount of effort required to remove them from the codebase is, IMO, unreasonably high. Keep the exceptions, most of the time they're used pretty judiciously.
* I really, really, *really* (really? Yes, really!) hate anything resembling Hungarian prefix notation, and have permanent scars from decades of reading it in Windows code. Please don't ask me to put a random 'k' in from of my enums - ick.
One other note: in the past, we've had conversations about "style only" pull requests to fix some of these things, and the guidance we ended up with has been to only fix this sort of thing while you're in the code working on a fix or a feature. I, for one, would welcome some PRs that just, say, renamed a ton of member variables to replace "m_" prefix with a simple trailing "_", perhaps fixed some of the more egregious and weird abbreviations, etc. My preference for bug fixes and feature work is that all of the code changes be focused on stuff that's relevant to the fix/feature, and mixing it with random style guide refactoring, I feel, muddies the waters for future maintainers.
Thanks,
Blake
-----Original Message-----
From: Jacob Barrett <ja...@vmware.com>
Sent: Saturday, May 1, 2021 9:21 AM
To: dev@geode.apache.org
Subject: Re: DISCUSSION: Geode Native C++ Style and Formatting Guide
Great call outs!
> On May 1, 2021, at 7:57 AM, Mario Salazar de Torres <ma...@est.tech> wrote:
>
> 1. Member variables names as of Google style guide requires a '_' char to be added at the end so it can be identified. Should we also adopt that?
> For example, imagine you have a region mutex, so, should we name it as 'regionMutex_' ?
I didn’t mention this one out in my review of differences because we are following it but I suppose with the combination of the camelCase difference we should probably call it out more specifically. Perhaps in our documentation we should show examples of both local and member variables. Do you think that will make it more clear?
> 2. Also, I would like to point out that macros are dis-recommended but every C++ committee member I know.
> What do you think about adding a notice saying: "Macros should be avoided and only used when there is no alternative”?
I think that is called out in various ways in a few places in the Google guide but I am more than happy for us to include strong or clearer language around this. Between constexpr and templates there are very cases for macros anymore.
We mostly use macros only to handle non-standard attributes. When we move to C++17 a lot of these will go away.
Thanks,
Jake
Re: DISCUSSION: Geode Native C++ Style and Formatting Guide
Posted by Jacob Barrett <ja...@vmware.com>.
> On May 3, 2021, at 11:23 AM, Blake Bender <bb...@vmware.com> wrote:
>
> My $0.02 on these:
>
> Things I'd like to see us conform to Google style on:
> * I'd be happy to move to C++ 17
This is likely an ABI change and certainly moves the minimum runtime library. The latter might be hard in a minor release.
> * Would also be happy to remove forward declarations. "I'm not a critic, but I know what I hate," as it were, and I hate forward declarations.
Yeah, the stance in Google C++ is don’t unless you really have to. We are more we do because we have include problems and it takes forever to build otherwise. We don’t use forwards in the public API though.
> * I would be very pleased to remove all the macros from our code. I've been bitten more than once in the past while debugging or refactoring our code, because of ill-formed macros.
There is no way to get rid of all of them. Our stance has been don’t do macros unless required, like platform/compiler specific attributes. As you find macros for just constants you should replace those. A lot of the large macros, like the old Cacheable stuff have been replaced with templates. There are only a few public macros, all of which are prefixed with GEODE.
> Google things I disagree with:
> * I don't like exceptions, but I don't even want to think about the amount of effort required to remove them from the codebase is, IMO, unreasonably high. Keep the exceptions, most of the time they're used pretty judiciously.
Yeah, no way we can change this.
> * I really, really, *really* (really? Yes, really!) hate anything resembling Hungarian prefix notation, and have permanent scars from decades of reading it in Windows code. Please don't ask me to put a random 'k' in from of my enums - ick.
I am on board with dropping the k if we want to diverge more from Google.
> One other note: in the past, we've had conversations about "style only" pull requests to fix some of these things, and the guidance we ended up with has been to only fix this sort of thing while you're in the code working on a fix or a feature. I, for one, would welcome some PRs that just, say, renamed a ton of member variables to replace "m_" prefix with a simple trailing "_", perhaps fixed some of the more egregious and weird abbreviations, etc. My preference for bug fixes and feature work is that all of the code changes be focused on stuff that's relevant to the fix/feature, and mixing it with random style guide refactoring, I feel, muddies the waters for future maintainers.
I don’t know where that idea that you can’t do style only changes came from. Do it. I suggest we just either have a format only JIRA or don’t require a JIRA for format only. Filing JIRAs for formatting changes is just noise.
-Jake
Re: DISCUSSION: Geode Native C++ Style and Formatting Guide
Posted by Jacob Barrett <ja...@vmware.com>.
> On May 4, 2021, at 7:59 AM, Mario Salazar de Torres <ma...@est.tech> wrote:
>
> Sure, gladly 🙂. Let me put up the info and I will open the discussion.
> And sorry for polluting this topic regarding Style and Formatting :S
No apologies! I just think this topic has a much bigger scope than documenting our deviations from the style. It has larger ramifications than say changing variable names. It is as discussion I am excited to help champion too.
Thanks,
Jake
Re: DISCUSSION: Geode Native C++ Style and Formatting Guide
Posted by Mario Salazar de Torres <ma...@est.tech>.
Sure, gladly 🙂. Let me put up the info and I will open the discussion.
And sorry for polluting this topic regarding Style and Formatting :S
Thanks,
Mario.
________________________________
From: Jacob Barrett <ja...@vmware.com>
Sent: Tuesday, May 4, 2021 4:42 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: DISCUSSION: Geode Native C++ Style and Formatting Guide
Mario,
Let’s start a separate discussion for going to C++17 on a minor release. Can you lay out the pros and cons to kick it off?
> On May 4, 2021, at 7:31 AM, Mario Salazar de Torres <ma...@est.tech> wrote:
>
> Hi everyone,
>
> Regarding Blake's comment on moving towards C++17 I completely agree.
> I think almost every compiler supports it now and given the latest changes on WoW regarding ABI,
> even if there were some ABI break (which is not the case from C++11 to C++17), everything should be fine.
>
> I'd really love to see C++20 as the standard for the project as it has several cool features, but sadly I must admit is too recent to be adopted.
>
> Thanks,
> Mario.
> ________________________________
> From: Blake Bender <bb...@vmware.com>
> Sent: Monday, May 3, 2021 8:23 PM
> To: dev@geode.apache.org <de...@geode.apache.org>
> Subject: RE: DISCUSSION: Geode Native C++ Style and Formatting Guide
>
> My $0.02 on these:
>
> Things I'd like to see us conform to Google style on:
> * I'd be happy to move to C++ 17
> * Would also be happy to remove forward declarations. "I'm not a critic, but I know what I hate," as it were, and I hate forward declarations.
> * I would also be happy with an 80-character line limit, though I don't feel strongly about it. 100 may be consistent with Geode, but it still feels arbitrary to me.
> * I would be very pleased to remove all the macros from our code. I've been bitten more than once in the past while debugging or refactoring our code, because of ill-formed macros.
>
> Google things I disagree with:
> * I don't like exceptions, but I don't even want to think about the amount of effort required to remove them from the codebase is, IMO, unreasonably high. Keep the exceptions, most of the time they're used pretty judiciously.
> * I really, really, *really* (really? Yes, really!) hate anything resembling Hungarian prefix notation, and have permanent scars from decades of reading it in Windows code. Please don't ask me to put a random 'k' in from of my enums - ick.
>
> One other note: in the past, we've had conversations about "style only" pull requests to fix some of these things, and the guidance we ended up with has been to only fix this sort of thing while you're in the code working on a fix or a feature. I, for one, would welcome some PRs that just, say, renamed a ton of member variables to replace "m_" prefix with a simple trailing "_", perhaps fixed some of the more egregious and weird abbreviations, etc. My preference for bug fixes and feature work is that all of the code changes be focused on stuff that's relevant to the fix/feature, and mixing it with random style guide refactoring, I feel, muddies the waters for future maintainers.
>
> Thanks,
>
> Blake
>
> -----Original Message-----
> From: Jacob Barrett <ja...@vmware.com>
> Sent: Saturday, May 1, 2021 9:21 AM
> To: dev@geode.apache.org
> Subject: Re: DISCUSSION: Geode Native C++ Style and Formatting Guide
>
> Great call outs!
>
>> On May 1, 2021, at 7:57 AM, Mario Salazar de Torres <ma...@est.tech> wrote:
>>
>> 1. Member variables names as of Google style guide requires a '_' char to be added at the end so it can be identified. Should we also adopt that?
>> For example, imagine you have a region mutex, so, should we name it as 'regionMutex_' ?
>
> I didn’t mention this one out in my review of differences because we are following it but I suppose with the combination of the camelCase difference we should probably call it out more specifically. Perhaps in our documentation we should show examples of both local and member variables. Do you think that will make it more clear?
>
>> 2. Also, I would like to point out that macros are dis-recommended but every C++ committee member I know.
>> What do you think about adding a notice saying: "Macros should be avoided and only used when there is no alternative”?
>
> I think that is called out in various ways in a few places in the Google guide but I am more than happy for us to include strong or clearer language around this. Between constexpr and templates there are very cases for macros anymore.
> We mostly use macros only to handle non-standard attributes. When we move to C++17 a lot of these will go away.
>
> Thanks,
> Jake
>
>
>
Re: DISCUSSION: Geode Native C++ Style and Formatting Guide
Posted by Jacob Barrett <ja...@vmware.com>.
Mario,
Let’s start a separate discussion for going to C++17 on a minor release. Can you lay out the pros and cons to kick it off?
> On May 4, 2021, at 7:31 AM, Mario Salazar de Torres <ma...@est.tech> wrote:
>
> Hi everyone,
>
> Regarding Blake's comment on moving towards C++17 I completely agree.
> I think almost every compiler supports it now and given the latest changes on WoW regarding ABI,
> even if there were some ABI break (which is not the case from C++11 to C++17), everything should be fine.
>
> I'd really love to see C++20 as the standard for the project as it has several cool features, but sadly I must admit is too recent to be adopted.
>
> Thanks,
> Mario.
> ________________________________
> From: Blake Bender <bb...@vmware.com>
> Sent: Monday, May 3, 2021 8:23 PM
> To: dev@geode.apache.org <de...@geode.apache.org>
> Subject: RE: DISCUSSION: Geode Native C++ Style and Formatting Guide
>
> My $0.02 on these:
>
> Things I'd like to see us conform to Google style on:
> * I'd be happy to move to C++ 17
> * Would also be happy to remove forward declarations. "I'm not a critic, but I know what I hate," as it were, and I hate forward declarations.
> * I would also be happy with an 80-character line limit, though I don't feel strongly about it. 100 may be consistent with Geode, but it still feels arbitrary to me.
> * I would be very pleased to remove all the macros from our code. I've been bitten more than once in the past while debugging or refactoring our code, because of ill-formed macros.
>
> Google things I disagree with:
> * I don't like exceptions, but I don't even want to think about the amount of effort required to remove them from the codebase is, IMO, unreasonably high. Keep the exceptions, most of the time they're used pretty judiciously.
> * I really, really, *really* (really? Yes, really!) hate anything resembling Hungarian prefix notation, and have permanent scars from decades of reading it in Windows code. Please don't ask me to put a random 'k' in from of my enums - ick.
>
> One other note: in the past, we've had conversations about "style only" pull requests to fix some of these things, and the guidance we ended up with has been to only fix this sort of thing while you're in the code working on a fix or a feature. I, for one, would welcome some PRs that just, say, renamed a ton of member variables to replace "m_" prefix with a simple trailing "_", perhaps fixed some of the more egregious and weird abbreviations, etc. My preference for bug fixes and feature work is that all of the code changes be focused on stuff that's relevant to the fix/feature, and mixing it with random style guide refactoring, I feel, muddies the waters for future maintainers.
>
> Thanks,
>
> Blake
>
> -----Original Message-----
> From: Jacob Barrett <ja...@vmware.com>
> Sent: Saturday, May 1, 2021 9:21 AM
> To: dev@geode.apache.org
> Subject: Re: DISCUSSION: Geode Native C++ Style and Formatting Guide
>
> Great call outs!
>
>> On May 1, 2021, at 7:57 AM, Mario Salazar de Torres <ma...@est.tech> wrote:
>>
>> 1. Member variables names as of Google style guide requires a '_' char to be added at the end so it can be identified. Should we also adopt that?
>> For example, imagine you have a region mutex, so, should we name it as 'regionMutex_' ?
>
> I didn’t mention this one out in my review of differences because we are following it but I suppose with the combination of the camelCase difference we should probably call it out more specifically. Perhaps in our documentation we should show examples of both local and member variables. Do you think that will make it more clear?
>
>> 2. Also, I would like to point out that macros are dis-recommended but every C++ committee member I know.
>> What do you think about adding a notice saying: "Macros should be avoided and only used when there is no alternative”?
>
> I think that is called out in various ways in a few places in the Google guide but I am more than happy for us to include strong or clearer language around this. Between constexpr and templates there are very cases for macros anymore.
> We mostly use macros only to handle non-standard attributes. When we move to C++17 a lot of these will go away.
>
> Thanks,
> Jake
>
>
>
Re: DISCUSSION: Geode Native C++ Style and Formatting Guide
Posted by Mario Salazar de Torres <ma...@est.tech>.
Hi everyone,
Regarding Blake's comment on moving towards C++17 I completely agree.
I think almost every compiler supports it now and given the latest changes on WoW regarding ABI,
even if there were some ABI break (which is not the case from C++11 to C++17), everything should be fine.
I'd really love to see C++20 as the standard for the project as it has several cool features, but sadly I must admit is too recent to be adopted.
Thanks,
Mario.
________________________________
From: Blake Bender <bb...@vmware.com>
Sent: Monday, May 3, 2021 8:23 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: RE: DISCUSSION: Geode Native C++ Style and Formatting Guide
My $0.02 on these:
Things I'd like to see us conform to Google style on:
* I'd be happy to move to C++ 17
* Would also be happy to remove forward declarations. "I'm not a critic, but I know what I hate," as it were, and I hate forward declarations.
* I would also be happy with an 80-character line limit, though I don't feel strongly about it. 100 may be consistent with Geode, but it still feels arbitrary to me.
* I would be very pleased to remove all the macros from our code. I've been bitten more than once in the past while debugging or refactoring our code, because of ill-formed macros.
Google things I disagree with:
* I don't like exceptions, but I don't even want to think about the amount of effort required to remove them from the codebase is, IMO, unreasonably high. Keep the exceptions, most of the time they're used pretty judiciously.
* I really, really, *really* (really? Yes, really!) hate anything resembling Hungarian prefix notation, and have permanent scars from decades of reading it in Windows code. Please don't ask me to put a random 'k' in from of my enums - ick.
One other note: in the past, we've had conversations about "style only" pull requests to fix some of these things, and the guidance we ended up with has been to only fix this sort of thing while you're in the code working on a fix or a feature. I, for one, would welcome some PRs that just, say, renamed a ton of member variables to replace "m_" prefix with a simple trailing "_", perhaps fixed some of the more egregious and weird abbreviations, etc. My preference for bug fixes and feature work is that all of the code changes be focused on stuff that's relevant to the fix/feature, and mixing it with random style guide refactoring, I feel, muddies the waters for future maintainers.
Thanks,
Blake
-----Original Message-----
From: Jacob Barrett <ja...@vmware.com>
Sent: Saturday, May 1, 2021 9:21 AM
To: dev@geode.apache.org
Subject: Re: DISCUSSION: Geode Native C++ Style and Formatting Guide
Great call outs!
> On May 1, 2021, at 7:57 AM, Mario Salazar de Torres <ma...@est.tech> wrote:
>
> 1. Member variables names as of Google style guide requires a '_' char to be added at the end so it can be identified. Should we also adopt that?
> For example, imagine you have a region mutex, so, should we name it as 'regionMutex_' ?
I didn’t mention this one out in my review of differences because we are following it but I suppose with the combination of the camelCase difference we should probably call it out more specifically. Perhaps in our documentation we should show examples of both local and member variables. Do you think that will make it more clear?
> 2. Also, I would like to point out that macros are dis-recommended but every C++ committee member I know.
> What do you think about adding a notice saying: "Macros should be avoided and only used when there is no alternative”?
I think that is called out in various ways in a few places in the Google guide but I am more than happy for us to include strong or clearer language around this. Between constexpr and templates there are very cases for macros anymore.
We mostly use macros only to handle non-standard attributes. When we move to C++17 a lot of these will go away.
Thanks,
Jake
RE: DISCUSSION: Geode Native C++ Style and Formatting Guide
Posted by Blake Bender <bb...@vmware.com>.
My $0.02 on these:
Things I'd like to see us conform to Google style on:
* I'd be happy to move to C++ 17
* Would also be happy to remove forward declarations. "I'm not a critic, but I know what I hate," as it were, and I hate forward declarations.
* I would also be happy with an 80-character line limit, though I don't feel strongly about it. 100 may be consistent with Geode, but it still feels arbitrary to me.
* I would be very pleased to remove all the macros from our code. I've been bitten more than once in the past while debugging or refactoring our code, because of ill-formed macros.
Google things I disagree with:
* I don't like exceptions, but I don't even want to think about the amount of effort required to remove them from the codebase is, IMO, unreasonably high. Keep the exceptions, most of the time they're used pretty judiciously.
* I really, really, *really* (really? Yes, really!) hate anything resembling Hungarian prefix notation, and have permanent scars from decades of reading it in Windows code. Please don't ask me to put a random 'k' in from of my enums - ick.
One other note: in the past, we've had conversations about "style only" pull requests to fix some of these things, and the guidance we ended up with has been to only fix this sort of thing while you're in the code working on a fix or a feature. I, for one, would welcome some PRs that just, say, renamed a ton of member variables to replace "m_" prefix with a simple trailing "_", perhaps fixed some of the more egregious and weird abbreviations, etc. My preference for bug fixes and feature work is that all of the code changes be focused on stuff that's relevant to the fix/feature, and mixing it with random style guide refactoring, I feel, muddies the waters for future maintainers.
Thanks,
Blake
-----Original Message-----
From: Jacob Barrett <ja...@vmware.com>
Sent: Saturday, May 1, 2021 9:21 AM
To: dev@geode.apache.org
Subject: Re: DISCUSSION: Geode Native C++ Style and Formatting Guide
Great call outs!
> On May 1, 2021, at 7:57 AM, Mario Salazar de Torres <ma...@est.tech> wrote:
>
> 1. Member variables names as of Google style guide requires a '_' char to be added at the end so it can be identified. Should we also adopt that?
> For example, imagine you have a region mutex, so, should we name it as 'regionMutex_' ?
I didn’t mention this one out in my review of differences because we are following it but I suppose with the combination of the camelCase difference we should probably call it out more specifically. Perhaps in our documentation we should show examples of both local and member variables. Do you think that will make it more clear?
> 2. Also, I would like to point out that macros are dis-recommended but every C++ committee member I know.
> What do you think about adding a notice saying: "Macros should be avoided and only used when there is no alternative”?
I think that is called out in various ways in a few places in the Google guide but I am more than happy for us to include strong or clearer language around this. Between constexpr and templates there are very cases for macros anymore.
We mostly use macros only to handle non-standard attributes. When we move to C++17 a lot of these will go away.
Thanks,
Jake
Re: DISCUSSION: Geode Native C++ Style and Formatting Guide
Posted by Jacob Barrett <ja...@vmware.com>.
Great call outs!
> On May 1, 2021, at 7:57 AM, Mario Salazar de Torres <ma...@est.tech> wrote:
>
> 1. Member variables names as of Google style guide requires a '_' char to be added at the end so it can be identified. Should we also adopt that?
> For example, imagine you have a region mutex, so, should we name it as 'regionMutex_' ?
I didn’t mention this one out in my review of differences because we are following it but I suppose with the combination of the camelCase difference we should probably call it out more specifically. Perhaps in our documentation we should show examples of both local and member variables. Do you think that will make it more clear?
> 2. Also, I would like to point out that macros are dis-recommended but every C++ committee member I know.
> What do you think about adding a notice saying: "Macros should be avoided and only used when there is no alternative”?
I think that is called out in various ways in a few places in the Google guide but I am more than happy for us to include strong or clearer language around this. Between constexpr and templates there are very cases for macros anymore.
We mostly use macros only to handle non-standard attributes. When we move to C++17 a lot of these will go away.
Thanks,
Jake
Re: DISCUSSION: Geode Native C++ Style and Formatting Guide
Posted by Mario Salazar de Torres <ma...@est.tech>.
Hi everyone,
Thanks, Jacob, for putting together a list of differences with the Google Style Guide for C++!
I just wanted to mention 2 things:
1. Member variables names as of Google style guide requires a '_' char to be added at the end so it can be identified. Should we also adopt that?
For example, imagine you have a region mutex, so, should we name it as 'regionMutex_' ?
2. Also, I would like to point out that macros are dis-recommended but every C++ committee member I know.
What do you think about adding a notice saying: "Macros should be avoided and only used when there is no alternative"?
Thanks,
Mario.
________________________________
From: Jacob Barrett <ja...@vmware.com>
Sent: Saturday, May 1, 2021 2:31 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: DISCUSSION: Geode Native C++ Style and Formatting Guide
Team,
We haven’t been good about documenting our style and formatting guidance. Like the Java sources are loosely styled after the Google Java Style Guide, the C++ sources are loosely styled after the Google C++ Style Guide. We deviate in some places from the Google C++ Style Guide. Without these documented it can be confusing to new or even infrequent committers. I would like us to discuss and then document our consensus in the CONTRIBUTING.md.
If you look at https://github.com/apache/geode-native/blob/develop/CONTRIBUTING.md#style you will notice there are no deviations documented.
To my recollection we have these deviations as common practice:
https://google.github.io/styleguide/cppguide.html#C++_Version
C++ Version
Currently, code should target C++11.
[ Google: Currently, code should target C++17. ]
https://google.github.io/styleguide/cppguide.html#Forward_Declarations
Forward Declarations
Forward declarations are allowed to reduce build time. Solving for complex includes should be preferred over this. See include what you use.
[ Google: Don’t use forward declarations ]
https://google.github.io/styleguide/cppguide.html#cpplint
cpplint
[Add] Prefer using clang-tidy and clang-format with the provide configuration files to validate style and format.
https://google.github.io/styleguide/cppguide.html#Exceptions
Exceptions
We use C++ exceptions.
[ Google: We do not use C++ exceptions. ]
https://google.github.io/styleguide/cppguide.html#Boost
Boost
Use only libraries from the Boost library collection when an alternative is not available in the standard library.
[ Google: Use only approved libraries from the Boost library collection. ]
https://google.github.io/styleguide/cppguide.html#File_Names
FIle Names
File names should match the case of the class declared within. C++ files should end in .cpp and their corresponding header should end with .hpp. Header only files may use .h extension.
https://google.github.io/styleguide/cppguide.html#Variable_Names
Variable Names:
Common Variable Names:
Use camelCase variable names.
[ Google: Use _ separated variable names. ]
https://google.github.io/styleguide/cppguide.html#Function_Names
Function Names:
Regular functions have camelCase; accessors and mutators may be named like variables.
[ Google: Regular functions have mixed case; accessors and mutators may be named like variables. ]
https://google.github.io/styleguide/cppguide.html#Macro_Names
Macro Names:
Prefix all macros with ‘GEODE'
[ Google: No prefix requirement ]
https://google.github.io/styleguide/cppguide.html#File_Comments
Legal Notice and Author Line
Every file should start with the Apache 2.0 license notice.
[ Google: Every file should contain license boilerplate. Choose the appropriate boilerplate for the license used by the project (for example, Apache 2.0, BSD, LGPL, GPL).]
No author name(s) should be used in the source.
[ Google: If you make significant changes to a file with an author line, consider deleting the author line. New files should usually not contain copyright notice or author line. ]
https://google.github.io/styleguide/cppguide.html#Class_Comments
Class Comments
[Add] Use Doxygen style comment format.
https://google.github.io/styleguide/cppguide.html#Function_Comments
Function Declarations
[Add] Use Doxygen style comment format.
https://google.github.io/styleguide/cppguide.html#Variable_Comments
[Add] Use Doxygen style comment format.
https://google.github.io/styleguide/cppguide.html#TODO_Comments
TODO Comments
TODOs should include the string TODO in all caps. TODOs should not contain any personal identifying information or bug ID.
[ Google: TODOs should include the string TODO in all caps, followed by the name, e-mail address, bug ID, or other identifier of the person or issue with the best context about the problem referenced by the TODO ]
These we should adopt but haven’t out of respect of previous convention:
https://google.github.io/styleguide/cppguide.html#Enumerator_Names
Enumerator Names:
Using Googles prescribed naming convention avoids collisions with leaky macros. I am indifferent to the use of the ‘k’ prefix but since they are constants it does make it consistent. We currently have a mix of camelCase and UPPERCASE enum constants.
Proposed changes we should adopt:
https://google.github.io/styleguide/cppguide.html#Line_Length
Line Length
Each line of text in your code should be at most 100 characters long.
Pro: Consistent with Geode Java Guide.
[ Google: Each line of text in your code should be at most 80 characters long. ]
Please comment or add anything I might have missed so we can get these recorded.
Thanks,
Jake