[gdal-dev] Possible code style changes to GDAL C++

[Even Rouault]

Kurt,

Thanks for bringing your propositions.

> With GDAL, I have to translate what I would want to do in C++11 to what
> older standards support.  That isn't easy.  I've also been trying to think
> about the time when GDAL decides to switch to C++11 or 14 as a minimum
> requirement.  How could code simplify, become more robust, as possibly
> faster if we had unique_ptr, make_unique, constexpr, static_assert, the new
> for loops over containers without having iterator noise, etc.

C++14 seems way too early due to the probable lack of broad compiler support.
We might consider C++11 at some point. Someone would have to review which 
C++11 compliant compilers are available on which platforms. For sure, MSVC 
2010 (used currently for OSGeo4W builds) must not be compliant as I discovered 
recently when compiling pdfium. 
We might have some pain with some proprietary SDKs shipping libraries for old 
MSVC versions, but they'll have to move along too at some point, if not 
already done. With that move, we might also left aside folks using proprietary 
Unix and their ancient & odd compilers, but I don't think we should care that 
much.

> 
> I've tried to make a start at a list of these, but I've not had time to put
> any reasonable discussion of the why.  One key goal in all this is not
> never impact the C ABI and try hard to not change the C++ ABI.
> 
> There really isn't an order to this...  The top priority is reducing the
> cognitive load on the reader.

Globally looks good to me (meaning +1 or +0 depending on items)

> 
> - Move variable declarations to first use (or as close to it as reasonable)
> - Initialize and declare in the same statement if possible
> - Add const when possible
> - Only declare one variable per line
> - Prefer C++ *_casts to C (int)foo casts or C++ int(foo)
> - Prefer bool/true/false to int/TRUE/FALSE when not changing an ABI
The downside is to have inconsistencies between "ABI related" code path using 
int/TRUE/FALSE and internal code using bool/true/false.
> - Prefer small blocks to returns first without scope for rest
>    if (poFoo == NULL)
>        return;
>    yada;
>    yada;
> - Declare loop variables in the for if at all possible
>   for( int i = 0; i < 10; ++i)
>     Foo(i);
>   not
>   int i;
>   for( i = 0; i < 123; ++i)
> - Prefer pre inc/dec if possible.  It's never slower than post.
> - Prefer a static const to a #defined const.
Would that also mean applying the usual rules for variable naming ? When I see 
ONE_MILE I know immediately it is a constant. dfOneMile would be less obvious.
> - Prefer a function to a #define macro if at all possible
> - Initialize all members in constructors for classes.  Also structs when
> not used by C code
> - Use std::fill + CPL_ARRAYSIZE over memset
> - Use ! to flip bools:  bFoo = !bFoo
> - Whitespace clean and new/modified code
> - Use NULL over 0 for pointers to make switching to nullptr easier.
> - Which is prefered for checking a nullptr?
>    if( poDS == NULL )
>    or
>    if( !poDS )
>    I see both.

Personnal preference:  "if( poDS == NULL )" is way more clearer. No ambiguity. 
When I see if (!poDS), I have to spend some time to realize it is not the same 
as "if( poDS != NULL)", and it is also easy to not notice the ! character if 
you read quickly.

> - Use correct grammar, capitalization and punctuation in comments.  Is
> British or American English prefered or no preference?

For a non native writer like me, no preference (I guess American English must 
be more common in software industry), but that rule would be somewhat hard to 
enforce.

> - Prefer C++ // to /* */
> - Except in unused arguments there int /* unused_foo */ is nice.
> - Drop my CPL_UNUSED macro for commenting out the name of the unused arg
> - Never use the non-length C string functions.  Use strncat (and write the
> \0), CPLsnprintf, etc.

We have also the CPLStrlcpy & CPLStrlcat that are the BSD versions. Not sure 
which ones are best. In most cases using std::string is possible and would 
avoid those headaches.

> - Do not check for a nullptr before calling CPLFree or for other functions
> that can handle a nullptr
> - Prefer initializer lists for constructors (ctors)
> - Keep member initialization in the same order as the class def even in the
> ctor body
> - Keep to the don't use exceptions model.  Many of us end up having to
> write catch wrappers that really suck, and the C interface can't expose
> exceptions.
> 
> Less clear:
> 
> - Wrap new code in namespaces.  Some of us are having a bad type with name
> collisions.  I propose osgeo::gdal as the root with the option to use more
> detailed namespaces for things like drivers.  Then we can stop worrying
> about more issues like I'm seeing between GDAL, KML and Poppler, where I'm
> having to use the C preprocesser to make generic function, class, and
> struct names not clash.

That may make sense in drivers yes.
The drawback I see is that it will make debugging with gdb harder due to extra 
typing for setting up breakpoints : "b 
osgeo::gdal::shape::OGRShapeDataSource::Open" yuck

> - If you can use a std::string and avoid CPLString, do so.
> - Prefer std::vector and std::map for non C facing internals rather than
> CSL* and the like
> - *New* code should be auto formatted by [pick some tool].  e.g. If I were
> to pick, it would be: clang-format -style="{BasedOnStyle: Google,
> IndentWidth: 4}", but anything that is consistent is great!   I've been
> spending a lot of time looking at the surrounding code and trying to figure
> how to match the local style.  It's exhausting when trying to read through
> large amounts of the code base with styles shifting all over the place.

Personaly I'm tolerant with small differences in styling, if the general 
appearance is OK. A space character that is here or not here before or after a 
parenthesis doesn't hurt my eyes ;-)
If there's consistent 4 space indentation and no tab, that's a lot.
I'm afraid that if we set too many rules, which require to have specific tools, 
occasional contributors will never manage to provide compliant code (from my 
experience, it is already very difficult to get code that is correct, compiles 
everywhere and has tests) and maintainers will have to suffer the additional 
pain.

> - Prefered way to write documentation in comments?  I see many styles.
> Which to pattern after in new code?
> - Can we pick a standard for flagging issues in the code?  Functions that
> are deprecated

There's a CPL_WARN_DEPRECATED macro available.

> or  coverity issues 

Does coverity recognize comment annotations to discard a false positive ? Or 
is it just in the UI ? Comment annotations would be great if the code is moved 
around.

> I don't know how to solve or known
> security issues that will break the ABI to fix (e.g. grib support).  I will
> also start to create bugs now that I'm getting past many of the really easy
> issues to solve.

A few additional thoughts:
- not sure if many contributors will have time & motivation to make existing 
code compliant to new rules.
- code churn might complicate backports in stable branches (and make results 
of 'svn blame' / 'git blame' more confusing), but that's expected.

Even

-- 
Spatialys - Geospatial professional services
http://www.spatialys.com