doc: make C++ core guidelines a reference

[joyeecheung]

As our C++ codebase consists of many code that interface with
third-party libraries that do not follow the C++ core guidelines,
it may be difficult to enforce the guidelines without sacrificing
consistency or introducing cruft that juggles with data structures.

This patch makes the C++ core guidelines a reference instead of a
general guideline to follow to reduce nits that are open to
interpretation during code reviews, and highlights the automatic
tooling for C++ styles. We may revisit the general priority when
there are better automatic tools in place.

[nodejs-github-bot]

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1582/pipeline

[refack]

I'm not in favor of the change as it is, I still think we can benefit for the C++CG. As I see it one of the benefits we get is not needing to explain why we do things when they fit the guide.
What I suggest is that we add a sentence about simply documenting places where we think the guidelines don't apply, and why.
IMHO the cost that will have by making us add more comments when writing code, will pay off when we or others read the code.

[refack]

In situations where following the guidelines is not the best way, a comment should be added explaining why. 

[joyeecheung]

one of the benefits we get is not needing to explain why we do things when they fit the guide.

Someone still has to read the guide, and rules still need to be interpreted, so without automatic tooling explanations are still needed until we all become good C++ core guideline lawyers (but even then there will be room for lawyering).

In situations where following the guidelines is not the best way, a comment should be added explaining why.

I don't think this would be sustainable without automatic tooling. The guideline should serve us, we do not serve the guidelines. If something like this cannot be easily done with something similar to a NOLINT comment, IMO it does not worth the additional code.

[Trott]

typo? should the Google?

[Trott]

(I guess follow is missing in the sentence?)

[Trott]

I'd drop In general if possible. It serves only to make following the style guide seem optional. Even if it is optional, should seems to provide sufficient wiggle-room for those times when you can't follow it.

[Trott]

may be -> are?

[refack]

I've been testing the available tools clang-tidy and msvs C++ core checker. Both seem to be doing a good job (no false positives, and run in reasonable time), the problem is they do the job too good, and we get 1000s of warnings. So we got a chicken and egg situation.
I'm trying to think how we can proceed from here. Any help or ideas will be appreciated.

[Trott]

I'm trying to think how we can proceed from here. Any help or ideas will be appreciated.

My idea is a generic one, but here you go anyway: Maybe an ad hoc working group (chartered or not)? Like, a small number of active contributors who are particularly active in the C++ code and who want the situation to improve. Have an actual discussion to devise an actual strategy. Prioritize what needs to be done first and how to do it. Also: Leverage Code & Learn and Node Todo to get broadly-supported practices implemented slowly if churn and review-ability of one mega-PR is a concern?

[addaleax]

👍

[fhinkel]

LGTM with comment's from Rich

[refack]

I strongly disagree with this change.

[refack]

I think this is a regression in the quality standards we strive to achieve. From my recept experience writing and reviewing code for this project, following the C++CG has unequivocally resulted in better code.

introducing cruft that juggles with data structures.

@joyeecheung, could you explain? AFAIK guidelines by definition eliminate cruft as they provide objective reasoning for each suggestion.

[joyeecheung]

@refack

could you explain? AFAIK guidelines by definition eliminate cruft as they provide objective reasoning for each suggestion.

For example when using the new v8::Array::New API with a known array of elements, with C-style arrays one just needs to

Local<Value> elements[] ={.... }; Array::New(isolate, elements, arraysize(elements));

Following the C++GC as cited here (albeit we were passing it into our own API in that specific piece of code):

std::array<Local<Value>, kKnownSize> elements ={.... }; Array::New(isolate, elements.data(), elements.size());

The C++CG generally prefers C++ STL containers over equivalent C constructs, but the third-party libraries don't necessarily accept those types - for instance, the new v8::Array::New API takes a pointer of Local<Value> and a length instead of std::vector or std::array for flexibility (note: Blink uses WTF::Vector instead of std::vector and that's the reason we went with that API design) so to construct an array in-place to pass to that API, if we are going to follow C++CG, we will need to wrap it in a std::array and that's unnecessary cruft in my opinion (we are not going to copy/assign it anywhere or iterating over it, we only need a pointer that points to a sequential block of Local<> pointers that gets used by a third-party library that does not take STL types because it serves a project that does not use STL at lot of the times).

The same can also be said about std::string v.s. Handle<String> types - in fact if we really want to optimize for our memory usage / add tracing for tracking memory corruption we may even try to use a home-grown allocator and that doesn't necessarily play along with the STL types (e.g. see the Zone types in V8) with the limited APIs they provide for custom allocations (unfortunately we can't just plug in another allocator in build time due to addon complexities - see #17007).

We don't necessarily have to go that far, for sure, but making C++CG a hard rule limits our possibilities and that's why I think it should be a reference instead of a law we need to follow and nit-picks with in code reviews. As far as I see C++CG is easier to follow when you are building a C++ project from the ground up, but not when you are gluing many third-party libraries together and have a compatibility burden.

[joyeecheung]

I think this is a regression in the quality standards we strive to achieve. From my recept experience writing and reviewing code for this project, following the C++CG has unequivocally resulted in better code.

There is a trade-off between regression in code quality and regression in the smoothness of code reviews - IMO we should refrain from nit-picking about stylish issues that do not affect functionality or performance in any significant way in code reviews, and dedicate cleanup patches to them. If we want to enforce a style, that can be automated instead of being enforced by human beings in code reviews, and the styles that can't be automated yet can be dealt with in separate patches. Having too many style nits is not healthy for code reviews, IMO, and it puts an emphasis on human effort when that style cannot be enforced with tooling, which is also unhealthy. We don't need every patch on the master to be perfect, especially in terms of style. As long as the tests pass (that includes linters, and potentially sanitizers when we can turn them on), we can always deal with style issues later.

[refack]

Let take the above example:
Classic C++

Local<Value> elements[] ={.... }; Array::New(isolate, elements, arraysize(elements));

Modern C++

std::array<Local<Value>, kKnownSize> elements ={.... }; Array::New(isolate, elements.data(), elements.size());

We can confirm that both generate exactly the same machine code - https://godbolt.org/z/-_J21G

As I see it the first snippet has two issue:

1. It introduces a hand rolled function arraysize, that we don't need.
2. passing Local<Value> elements[] to a function that takes Local<Value>* is describe as "decay". The compiler can't guarantee that information (size) is not lost. C++ considers this a footgun, but it's there for legacy.

The second snippet has one issue:

1. (For now) we need to to explicitly state <Local<Value>, kKnownSize>. This is solved is C++17. But we made the argument that in general we prefer explicitness anyway (doc: formalize auto usage in C++ style guide #23028)

The bonus is that we do have "decay" we explicitly have to call elements.data() telling the compiler "I'm sure, this is what I want to happen!".

As for the smoothness of code reviews, IMHO that is one of the great benefits of this project. It's a place were we all can learn and teach each other. We should celebrate comments that makes us think and reconsider what we are writing.

[bnoordhuis]

Since @refack's nack still stands, does this need to be put to the TSC? I don't think we actually follow the Core Guidelines all that much so in that respect the style guide is currently at odds with reality.

[refack]

I'd be happy to hear new perspectives after 3 months.
Since AFAICT we anyway treat this document as a non blocking guideline, and code that does not conform even to the explicitly stated paragraphs can land, I see no real need to change it ¯\(ツ)/¯

[bnoordhuis]

Another six months passed. I would like for this to either go in or be closed out.

@refack Do you still have a vested interest in this?

[fhinkel]

I'm cleaning out a few old PRs 🧹. I'm closing this due to inactivity. Please re-open if needed!