<div dir="ltr">Hi all,
<div><br></div><div>TL;DR: This is a list of possible topics for change to <a href="https://trac.osgeo.org/gdal/wiki/rfc8_devguide">RFC 8</a>. 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.</div><div><br></div><div>Cheers,</div><div>-kurt</div><div><br></div><div><br></div><div>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 <a href="https://trac.osgeo.org/gdal/wiki/rfc8_devguide">RFC 8: Developer Guidelines</a>. 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. </div><div><br></div><div>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 <a href="https://google-styleguide.googlecode.com/svn/trunk/cppguide.html">Google C++ Style Guide</a> (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 <a href="https://github.com/bazelbuild/bazel">bazel</a>). </div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>There really isn't an order to this... The top priority is reducing the cognitive load on the reader.</div><div><br></div><div><div>- Move variable declarations to first use (or as close to it as reasonable)</div><div>- Initialize and declare in the same statement if possible</div><div>- Add const when possible</div><div>- Only declare one variable per line</div><div>- Prefer C++ *_casts to C (int)foo casts or C++ int(foo)</div><div>- Prefer bool/true/false to int/TRUE/FALSE when not changing an ABI</div><div>- Prefer small blocks to returns first without scope for rest</div><div> if (poFoo == NULL)</div><div> return;</div><div> yada;</div><div> yada;</div><div>- Declare loop variables in the for if at all possible</div><div> for( int i = 0; i < 10; ++i)</div><div> Foo(i);</div><div> not</div><div> int i;</div><div> for( i = 0; i < 123; ++i)</div><div>- Prefer pre inc/dec if possible. It's never slower than post.</div><div>- Prefer a static const to a #defined const.</div><div>- Prefer a function to a #define macro if at all possible</div><div>- Initialize all members in constructors for classes. Also structs when not used by C code</div><div>- Use std::fill + CPL_ARRAYSIZE over memset</div><div>- Use ! to flip bools: bFoo = !bFoo</div><div>- Whitespace clean and new/modified code</div><div>- Use NULL over 0 for pointers to make switching to nullptr easier.</div><div>- Which is prefered for checking a nullptr?</div><div> if( poDS == NULL )</div><div> or </div><div> if( !poDS )</div><div> I see both.</div><div>- Use correct grammar, capitalization and punctuation in comments. Is British or American English prefered or no preference?</div><div>- Prefer C++ // to /* */</div><div>- Except in unused arguments there int /* unused_foo */ is nice.</div><div>- Drop my CPL_UNUSED macro for commenting out the name of the unused arg </div><div>- Never use the non-length C string functions. Use strncat (and write the \0), CPLsnprintf, etc.</div><div>- Do not check for a nullptr before calling CPLFree or for other functions that can handle a nullptr</div><div>- Prefer initializer lists for constructors (ctors)</div><div>- Keep member initialization in the same order as the class def even in the ctor body</div><div>- 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.</div><div><br></div><div>Less clear:</div><div><br></div><div>- 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.</div><div>- If you can use a std::string and avoid CPLString, do so.</div><div>- Prefer std::vector and std::map for non C facing internals rather than CSL* and the like</div></div><div>- *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.</div><div>- Prefered way to write documentation in comments? I see many styles. Which to pattern after in new code?</div><div><div>- 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.</div></div><div><br></div>
<div><br></div><div><br></div><div><br></div></div>