Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

c++: Adopt the CppCoreGuidelines #22862

Closed
refack opened this issue Sep 14, 2018 · 5 comments · Fixed by #23317
Closed

c++: Adopt the CppCoreGuidelines #22862

refack opened this issue Sep 14, 2018 · 5 comments · Fixed by #23317
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. discuss Issues opened for discussions and feedbacks.

Comments

@refack
Copy link
Contributor

refack commented Sep 14, 2018

Now that we build with C++14 enabled:

I believe it's time we start adopting the CppCoreGuidelines
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines

@refack refack added c++ Issues and PRs that require attention from people who are familiar with C++. discuss Issues opened for discussions and feedbacks. labels Sep 14, 2018
@richardlau
Copy link
Member

We still use -std=gnu++0x on v8.x -- Are the guidelines compatible (bearing in mind backports)?

@addaleax
Copy link
Member

This seems to be a very broad guide on writing good C++, so what exactly does “adopt” mean here? Also, some of these are incompatible with our own existing guidelines (e.g. use of non-const references).

@refack
Copy link
Contributor Author

refack commented Sep 14, 2018

From a community POV, having an explicit and concise "guidebook" to C++ can be a huge benefit for newcomers (and current contributors who want to expand to C++). Especially since C++ is so expressive and flexible, even after learning the language knowing what is the right way to do something is very hard.

I don't believe we need/can/should refactor the existing code. IMHO it's similar to when we discussed ES-next, i.e. when writing new code or fixing existing code, CCG guidelines should take precedence.


We still use -std=gnu++0x on v8.x -- Are the guidelines compatible (bearing in mind backports)?

@richardlau, most of the clauses in the CCG are stylistic, and do not necessarily require new language features. They do at times recommend new features, but explain why, and how to handle legacy situations.

We need as always to balance comfort and progress with code backportability.
v8 will enter maintenance in ~6 months, so it's not that far away. Also in C++ code we have the compiler (not just git) to verify that backported changes are consistent.


This seems to be a very broad guide on writing good C++, so what exactly does “adopt” mean here?

@addaleax IMHO “adopt” means accepting those guidelines as the default. Reviewing PRs with the CCG in mind, and requesting explanations when authors overrules them.
There are also tools (MSVS, and clang-tidy) that help check and flag situations that are covered by the CCG. We could incrementally add those.

Also, some of these are incompatible with our own existing guidelines (e.g. use of non-const references).

A quote from the Google Style guide: The C++ version targeted by this guide will advance (aggressively) over time. We could agree that some of our current guidelines need to be updated.
At least from what I read, every clause in the guide comes with reasonings, pro and con. IMHO the ISO committee and the people behind the CCG are some of the most experienced in the field, so I'm assuming their reasoning is sound.

@addaleax
Copy link
Member

How about we start by adding this as a recommendation to read/look up from our style guide file? It’s a very comprehensive work, so while it certainly can help newcomers, we can’t expect them to be familiar with all of it.

@refack
Copy link
Contributor Author

refack commented Nov 13, 2018

Fixed by 0f8eaa4 (#23317)

@refack refack closed this as completed Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. discuss Issues opened for discussions and feedbacks.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants