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

[Kurt Schwehr]

Hi all,

TL;DR: This is a list of possible topics for change to RFC 8
<https://trac.osgeo.org/gdal/wiki/rfc8_devguide>.  I think the default
action should be to not to add / change anything.  If a point is worth
discussing, we should probably create a bug for just that point and discuss
it there.  Remember, this is a topic where there is usually many good
answers that often conflict.

Cheers,
-kurt


Most of you have probably notice the very large number of patches I've been
making to GDAL to work through the backlog of Coverity Scan complaints (we
still have >500 in the general scan and > 1000 in kssshannon's more
aggressive coverage).  While in the code, I've been trying to make the code
(I hope!) more readable when it was easy to do.  I also think these changes
will make it easier to follow the code when in non-optimized mode.  Even
thought it might be worth discussing and seeing if any of these should be
incorporated into RFC 8: Developer Guidelines
<https://trac.osgeo.org/gdal/wiki/rfc8_devguide>.  With GDAL, this is a
real challenge.  We all come from very different backgrounds and have
different requirements for GDAL.  And as seen across the internet, there
are lots of very good C++ style guides with orthogonal / conflicting
opinions.

For a bit of context for me that is worth having in mind when reading this
initial list.  First, I really started doing C++ in the GCC 2.7.2 / SGI
Compiler in the 1996-8 timeframe.  Back then, STL was new and the compilers
really sucked and the support tools were horrible (CVS, no CI, Purify was
99.9 noise, etc).  I've been the fink (debian packaging on mac osx) for a
decade.  For the last couple years, I've been working in the internal
Google code base with > 10K engineers in a single code base and working in
C++11 with being required to meet the Google C++ Style Guide
<https://google-styleguide.googlecode.com/svn/trunk/cppguide.html> (the
internal one is even more detailed and is more of a living document).
Continual Integration testing is the center of the universe, testing is
done with gunit for C++ and Python's unittest2, and building is done with
the Google build system (open sourced as bazel
<https://github.com/bazelbuild/bazel>).
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.

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.

- 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
- 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.
- 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.
- Use correct grammar, capitalization and punctuation in comments.  Is
British or American English prefered or no preference?
- 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.
- 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.
- 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.
- 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 or  coverity issues 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osgeo.org/pipermail/gdal-dev/attachments/20150920/932a8e92/attachment-0001.html>