mapbox / vtquery

Query some gosh darn vector tiles
BSD 2-Clause "Simplified" License
44 stars 15 forks source link

Code consisitency/correctness around pointers #96

Open springmeyer opened 6 years ago

springmeyer commented 6 years ago

In regards to https://github.com/mapbox/vtquery/blob/5ffe8d7dac2851031b873ba088a27f1ae5add78a/src/vtquery.cpp#L26-L29

@alliecrevier ask:

[nitpick] Since T const& instead of const T& is the style used throughout this code file, it might make sense to use this style for pointers as well. So instead of const char and static const char you would use char const and static char const respectively. It looks weird to me but it's consistent!

joto commented 6 years ago

There is a huge controversy in the C++ community on this at the moment. This is now called "east const" vs. "const west". At the last big C++ conference (C++Now 2018) supporters of both sides gave out colored armbands to show support for on side or the other. Here is one blog post about this: http://slashslash.info/2018/02/a-foolish-consistency/

Anyway, consistency is good, so either one or the other.

sssoleileraaa commented 6 years ago

Looks like "const west" people won: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#nl26-use-conventional-const-notation. But at least the "east const" people knew exactly which arm to wear the armband on.

(the right arm of course)

joto commented 6 years ago

@alliecrevier No, those core guidelines were written before this discussion blew up in the last few months and they are not the result of some kind of community consensus. The guidelines were critized in the discussion for using the wrong arguments. So this is still ongoing.

sssoleileraaa commented 6 years ago

@joto What do you think about using the C++ Core Guidelines except when we have reason to disagree?

As a way of making it easier to share and contribute to code in a consistent way, we can point people to the C++ Core Guidelines but tell them to check the Mapbox C++ Core-tech Guidelines for exceptions. Each project could even have their own set of guideline exceptions.

This could be a convenient way to organize our style guide. For instance, if we want to document that we use "east const" it could look something like:

NL.26-modified: Use east const notation

Reason Even though conventional notation, aka const west notation, is more familiar to more programmers, we believe east const improves the readability of our code because it's simpler. The rule is that const should always be on the right of what it modifies.

Example

int const y = 9;  // recommended, const int
const int x = 7;  // not recommended, const int
int const * p = nullptr;  // recommended, pointer to const int
const int * p = nullptr;  // not recommended, pointer to const int
int * const  p = nullptr; // recommended (for both notations), const pointer to int
int const * const p = nullptr;  // recommended, const pointer to const int
const int * const p = nullptr;  // not recommended, const pointer to const int

Note The recommended examples follow a simple rule so is easier to read and teach.

joto commented 6 years ago

Using the C++ Core Guidelines as a base and building on it is a good idea. Most of the Core Guidelines are uncontroversial and really good even if they are somewhat geared towards higher level code (pointer arithmetic is frowned upon for instance which is needed for low-level code).

But this disucssion has gone a "little" beyond what this issue was originally about and probably needs to be discussed somewhere else.