Discussion:
Starting a discussion on style and coding guidelines
(too old to reply)
Kurt Schwehr
2016-05-04 22:30:01 UTC
Permalink
Hi all,

If you've been watching the timeline on trac, you have probably seen a
large number of cleanup CLs from me. It's definitely past time to get some
discussion going on these changes. If the community likes these, we can
add them to rfc8_devguide <https://trac.osgeo.org/gdal/wiki/rfc8_devguide>.
We also need to consider tools that help committers push code towards what
we want and keep it there once it conforms to those guides (e.g.
clang-format with a custom GDAL specification) If you haven't see one of
the changes I've committed yet, here is an example to check out... this
change to geotiff.cpp is likely the biggest I will ever do.

https://trac.osgeo.org/gdal/changeset/34130/

I personally think the goals of style / coding guidelines are:

0. Ensure that GDAL is around for a long time to come
1. Make the code easier to read (which particular patterns to use and
consistent formatting)
2. When the code has trouble or data is bad, fail with error messages
rather than crash
3. Make it easier for debuggers to present sensible state
4. Help static (clang static analyzer, Coverity, etc. ) and dynamic (e.g.
fuzzers) analyzers perform better
5. Make it easier for authors to contribute code that plays well with the
rest of system with the least possible effort

To start off the conversation, I wrote up a doc on changing large C arrays
on the stack to std::vectors to get this data off of the stack and to
simplify initialization. I've attached the doc inline, but it is also
available here:

goo.gl/vuA3D6

The full url is:
https://docs.google.com/document/d/1Y9flzxj3Uz1vTEPCBmlswgi470m8i-oepGutVkbowfc/pub

It would be great to get people to have at commenting on this proposal. If
you all like this kind of thing, we can more of them and work them into a
form that extends RFC 8. Please don't be shy in commenting any way you
feel is constructive on this doc. We can discuss on this list or if people
really want, I can give individuals comment access to the Google doc.

cheers,
-kurt


Use vector<T>(length, initial_value) for local blocks of storage.

Author: Kurt Schwehr ***@google.com / ***@gmail.com (goatbar)

Date: 2016-May-04

Consider:

int anVals[256];

memset(anVals, 0, 256*sizeof(int));

This is less than optimal because:


-

It puts 1K or 2K on the stack, some of us (who can have huge numbers of
threads) try to keep stack usage to 16K per thread
-

You have two places that need to know the size
-

The variable exists uninitialized between the definition and
initialization
-

memset is a C API, which is able to do fewer sanity checks
-

No bounds checking


In GDAL, there are >700 stack allocations of more than 100 items and > 150
of 1000 or more items. There are 877 calls to memset in the C++ code.

Proposed solution:

https://trac.osgeo.org/gdal/changeset/34177

std::vector<int> oVals(256, 0);

Benefits:


-

It can pretty much be used like a C array, e.g., ++anVals[index]; works
-

Using vector gets most of the memory off of the stack and into the heap
helping threaded programs
-

The values are all initialized when anVals is created
-

If functions need to get a pointer to a C style array, you can call
Foo(&anVals[0])
-

Compilers can optimize the standard containers.
-

Bounds checking is possible without having to resort to ASAN (which not
all compilers have) or Valgrind


Drawbacks:


-

It is possible to change the size of the vector later on in the code
-

Vector has some storage overhead and bookkeeping that has to be done
(but often the compiler can probably optimize away most of that). TODO:
References that explain this?
-

Resizing the array could break anything that was using a C style array
pointer to the vector’s data


Alternatives

Might be better or worse in different ways:


-

Use std::fill instead of memset
-

Declare and pass around the array size as a constant, e.g., const int
knValsSize = 256.
-

CPL_ARRAY_SIZE to only have one 256 around
-

Use CPLCalloc to put it on the heap and initialize
-

Use new[] to put it on the heap
-

Use std::array <http://www.cplusplus.com/reference/array/>. Introduced
in C++ 11, so currently disallowed. Still puts the payload on the stack.
-

Use std::unique_ptr <http://en.cppreference.com/w/cpp/memory/unique_ptr>
to put it on the heap. Introduced in C++ 11, so currently disallowed
-

Add autoptr to GDAL to put it on the heap. autoptr is very similar to
unique_ptr, but works with C++03
-

Use some other library like boost. That would require a major change to
GDALs build requirements.
-

int anVals[256] = { 0 }; Only initializes the first element or you must
specify all 256 elements.
-

Make our own simple class that handles allocation on the heap and cleans
up in the destructor and provides operator[]


Key GDAL facts to keep in mind:

-

Only C++03 is currently allowed
-

Want to focus on maintainability and readability of code
-

A very diverse set of people work on the GDAL code base
-

Currently support a wide range of compilers / platforms:
-

32 and 64 bit
-

Linux, Android, MacOSX, Windows >= Win7, Cigwin
-

Big and little endian
-

GCC 4.8, GCC 5.2, MingW, VC9, VC12, VC13
-

Build with C++03, C++11, C++14
-

Build with C89, C99, C11
-

Python 2.7 and >= 3.5 (we might actually support 2.6 and 3.4,
Idonno)
-

https://travis-ci.org/rouault/gdal_coverage/builds/
-

https://ci.appveyor.com/project/rouault/gdal-coverage/history
-

Want to help Coverity, Address Sanitizer, AFL, Clang Static Analyzer
-

GDAL uses a form of


What if ...:

GDAL goes with C++11 or 14? It would probably be best to stick with the
vector solution as it is simpler than unqiue_ptr and std::array still puts
a large amount of data off the stack. vector is still the simplest with
initialization.

GDAL does not care about stack size and C++14? You could do a unqiue_ptr
with a CPLCalloc and a deleter of CPLFree.


-

std::unique_ptr<std::array<int, 256>> Vals(new std::array<int, 256>());
-

std::unique_ptr<int *, std::function<void(char *)>> Vals(CPLCalloc(256,
0), CPLFree);
-

std::unique_ptr<int *, CplFreeDeleter> Vals( CPLCalloc(256, 0), CPLFree);
-

// Function object that calls GDAL's CPLFree on its parameter.
struct CplFreeDeleter
<https://cs.corp.google.com/piper///depot/google3/third_party/gdal/autotest2/cpp/util/cpl_memory.h?l=10&gs=cpp%253Aautotest2%253A%253Aclass-CplFreeDeleter%2540google3%252Fthird_party%252Fgdal%252Fautotest2%252Fcpp%252Futil%252Fcpl_memory.h%257Cdef&gsn=CplFreeDeleter&ct=xref_usages>
{
void operator()
<https://cs.corp.google.com/piper///depot/google3/third_party/gdal/autotest2/cpp/util/cpl_memory.h?l=11&gs=cpp%253Aautotest2%253A%253Aclass-CplFreeDeleter%253A%253Aoperator()(void%2B*)%2540google3%252Fthird_party%252Fgdal%252Fautotest2%252Fcpp%252Futil%252Fcpl_memory.h%257Cdef&gsn=operator()&ct=xref_usages>
(void*
<https://cs.corp.google.com/piper///depot/google3/GENERATED/figments/cpp/PointerTo/void.cc?l=3&ct=xref_jump_to_def&cl=GROK&gsn=*>
ptr
<https://cs.corp.google.com/piper///depot/google3/third_party/gdal/autotest2/cpp/util/cpl_memory.h?l=11&gs=cpp%253Aautotest2%253A%253Aclass-CplFreeDeleter%253A%253Aoperator()(void%2B*)%253A%253Aparam-ptr%2540google3%252Fthird_party%252Fgdal%252Fautotest2%252Fcpp%252Futil%252Fcpl_memory.h%257Cdef&gsn=ptr&ct=xref_usages>)
{ CPLFree
<https://cs.corp.google.com/piper///depot/google3/blaze-out/gcc-4.X.Y-crosstool-v18-hybrid-grtev4-k8-fastbuild/include/third_party/gdal/_/gdal/third_party/gdal/port/cpl_conv.h?l=72&ct=xref_jump_to_def&cl=GROK&gsn=CPLFree>
(ptr
<https://cs.corp.google.com/piper///depot/google3/third_party/gdal/autotest2/cpp/util/cpl_memory.h?l=11&ct=xref_jump_to_def&cl=GROK&gsn=ptr>);
}
};
-

std::unique_ptr<int[]> Vals(new int[256]);


Open questions:

-

At what size do we want to make as the initial threshold to recommend
heap verses stack? 32 bytes seems like no big deal on the stack and 512
bytes seems like it really needs to be on the heap without a specific
reason.


See also:

https://trac.osgeo.org/gdal/ticket/5748 Reduce GDAL stack usage

https://google.github.io/styleguide/cppguide.html#Local_Variables

http://stackoverflow.com/questions/22571052/replace-fixed-size-arrays-with-stdarray

http://stackoverflow.com/questions/15294129/overhead-to-using-stdvector

http://www.cplusplus.com/reference/memory/unique_ptr/operator[]/
<about:blank>

http://stackoverflow.com/questions/16711697/is-there-any-use-for-unique-ptr-with-array

Acknowledgements:

Several Google engineers commented on an initial draft of this doc.
Mateusz Loskot
2016-05-04 22:51:28 UTC
Permalink
Post by Kurt Schwehr
To start off the conversation, I wrote up a doc on changing large C arrays
on the stack to std::vectors to get this data off of the stack and to
simplify initialization.
Since many, if not most, of the ideas rely on availability of the C++11
features,
it might be a good idea to first agree on C++11 as the lowest
required C++ compilation mode.

Let's poll if there are any users who require C++03 at all.

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
Even Rouault
2016-05-04 23:45:49 UTC
Permalink
Post by Mateusz Loskot
Post by Kurt Schwehr
To start off the conversation, I wrote up a doc on changing large C
arrays on the stack to std::vectors to get this data off of the stack
and to simplify initialization.
Since many, if not most, of the ideas rely on availability of the C++11
features,
it might be a good idea to first agree on C++11 as the lowest
required C++ compilation mode.
Let's poll if there are any users who require C++03 at all.
I think there are different topics in what Kurt proposes :
- style and other changes that are not bound to a particular compiler version
- changes potentially dependant of the C++ version

Of the potential issues with requiring C++11, I can think of OSGeo4W. It is
mostly(completely?) built with Visual Studio 2010. And from
https://msdn.microsoft.com/en-us/library/hh567368.aspx , support of C++11 is
only partial in VS 2010

Regarding the current use of VS2010 in OSGeo4W, this is discussed here :
https://lists.osgeo.org/pipermail/discuss/2016-February/015658.html

Potentially we could imagine having GDAL compiled with a later VS version and
have the rest using VS2010, since most (all?) other software in OSGeo4W use it
probably through its C API. Hum, actually that must not be true for OTB that
uses the C++ API I think. Perhaps Jürgen has something to add regarding this
compiler issue ?

On the other hand regarding dependencies of GDAL, the binary propritary SDKs
with a C++ API could be a problem, although they will likely move on too.
- FileGDB SDK 1.4: available for VS2010, VS2012, VS2013
- ECW SDK 5.2.1: available for VS2010, VS2012, VS2013
- MrSID SDK: I didn't check. Perhaps Kirk can tell us ?
But I'm not sure about the compatibility of C++11 build against non-C++11
builds in the VS realm : can a GDAL C++11 build link against a library built
without C++11 enabled ? Will not there be ABI problems ?

Even
--
Spatialys - Geospatial professional services
http://www.spatialys.com
Dmitry Baryshnikov
2016-05-05 08:14:56 UTC
Permalink
Hi,

I think it's time to go forward and shift to C++11 in i.e. GDAL 2.2 and
drop old staff as it was with Windows mobile support, VB, etc.
Yes, it may be some regression in ABI, but this is less evil than
support lot of ancient compilers.

During our work on cmake build system for GDAL I faced that some
dependency library needs VC2013SP2 on Windows - i.e liblzma, as it needs
C99.
We worked around it by including some logic to cmake, that if VC
compiler is less than specific version the support of dependent features
will be disabled.

By the way, the repo with cmake for GDAL is here:
https://github.com/nextgis-extra/lib_gdal
The ubuntu ppa of libgdal2 build using cmake:
https://launchpad.net/~nextgis/+archive/ubuntu/ppa/+packages
Windows builds also using cmake: http://nextgis.ru/en/borsch/

Please note, that most but not all drivers are available now (for
details see readme in repo) and only python bindings.

Best regards,
Dmitry
Post by Even Rouault
Post by Mateusz Loskot
Post by Kurt Schwehr
To start off the conversation, I wrote up a doc on changing large C
arrays on the stack to std::vectors to get this data off of the stack
and to simplify initialization.
Since many, if not most, of the ideas rely on availability of the C++11
features,
it might be a good idea to first agree on C++11 as the lowest
required C++ compilation mode.
Let's poll if there are any users who require C++03 at all.
- style and other changes that are not bound to a particular compiler version
- changes potentially dependant of the C++ version
Of the potential issues with requiring C++11, I can think of OSGeo4W. It is
mostly(completely?) built with Visual Studio 2010. And from
https://msdn.microsoft.com/en-us/library/hh567368.aspx , support of C++11 is
only partial in VS 2010
https://lists.osgeo.org/pipermail/discuss/2016-February/015658.html
Potentially we could imagine having GDAL compiled with a later VS version and
have the rest using VS2010, since most (all?) other software in OSGeo4W use it
probably through its C API. Hum, actually that must not be true for OTB that
uses the C++ API I think. Perhaps Jürgen has something to add regarding this
compiler issue ?
On the other hand regarding dependencies of GDAL, the binary propritary SDKs
with a C++ API could be a problem, although they will likely move on too.
- FileGDB SDK 1.4: available for VS2010, VS2012, VS2013
- ECW SDK 5.2.1: available for VS2010, VS2012, VS2013
- MrSID SDK: I didn't check. Perhaps Kirk can tell us ?
But I'm not sure about the compatibility of C++11 build against non-C++11
builds in the VS realm : can a GDAL C++11 build link against a library built
without C++11 enabled ? Will not there be ABI problems ?
Even
Jeff McKenna
2016-05-05 12:10:55 UTC
Permalink
Post by Dmitry Baryshnikov
Hi,
I think it's time to go forward and shift to C++11 in i.e. GDAL 2.2 and
drop old staff as it was with Windows mobile support, VB, etc.
Yes, it may be some regression in ABI, but this is less evil than
support lot of ancient compilers.
During our work on cmake build system for GDAL I faced that some
dependency library needs VC2013SP2 on Windows - i.e liblzma, as it needs
C99.
We worked around it by including some logic to cmake, that if VC
compiler is less than specific version the support of dependent features
will be disabled.
https://github.com/nextgis-extra/lib_gdal
https://launchpad.net/~nextgis/+archive/ubuntu/ppa/+packages
Windows builds also using cmake: http://nextgis.ru/en/borsch/
About a month ago I made the hard decision to upgrade my entire MS4W
build environment from VC 2008 to 2015. Driving this is that PHP 7 only
supports 2015.

The only GDAL dependent library not supported for VC 2015 is FileGDB
(I've seen an Esri developer state that 2015 SDK will be released with
their next FileGDB API upgrade).

MrSID 9.5.1 SDK works well with 2015.

I didn't try an ECW 5.x SDK, but the old 3.3 SDK works well with 2015.

Short story is I support future GDAL builds requiring a more recent
Visual Studio compiler (2013+), as other projects are already doing so.

For those packagers cringing in the thought of upgrading their whole
stack: join the club :)

-jeff
--
Jeff McKenna
MapServer Consulting and Training Services
http://www.gatewaygeomatics.com/
Jeff McKenna
2016-05-05 12:22:03 UTC
Permalink
Post by Jeff McKenna
Post by Dmitry Baryshnikov
Hi,
I think it's time to go forward and shift to C++11 in i.e. GDAL 2.2 and
drop old staff as it was with Windows mobile support, VB, etc.
Yes, it may be some regression in ABI, but this is less evil than
support lot of ancient compilers.
During our work on cmake build system for GDAL I faced that some
dependency library needs VC2013SP2 on Windows - i.e liblzma, as it needs
C99.
We worked around it by including some logic to cmake, that if VC
compiler is less than specific version the support of dependent features
will be disabled.
https://github.com/nextgis-extra/lib_gdal
https://launchpad.net/~nextgis/+archive/ubuntu/ppa/+packages
Windows builds also using cmake: http://nextgis.ru/en/borsch/
About a month ago I made the hard decision to upgrade my entire MS4W
build environment from VC 2008 to 2015. Driving this is that PHP 7 only
supports 2015.
The only GDAL dependent library not supported for VC 2015 is FileGDB
(I've seen an Esri developer state that 2015 SDK will be released with
their next FileGDB API upgrade).
MrSID 9.5.1 SDK works well with 2015.
I didn't try an ECW 5.x SDK, but the old 3.3 SDK works well with 2015.
Short story is I support future GDAL builds requiring a more recent
Visual Studio compiler (2013+), as other projects are already doing so.
For those packagers cringing in the thought of upgrading their whole
stack: join the club :)
I should also note that for compiling Python 3.5.x VC 2015 is the
supported environment. Yet another GDAL dependent library that favors
an upgraded compiler on Windows.

-jeff
--
Jeff McKenna
MapServer Consulting and Training Services
http://www.gatewaygeomatics.com/
Even Rouault
2016-05-05 13:26:33 UTC
Permalink
Post by Jeff McKenna
Post by Jeff McKenna
Post by Dmitry Baryshnikov
Hi,
I think it's time to go forward and shift to C++11 in i.e. GDAL 2.2 and
drop old staff as it was with Windows mobile support, VB, etc.
Yes, it may be some regression in ABI, but this is less evil than
support lot of ancient compilers.
During our work on cmake build system for GDAL I faced that some
dependency library needs VC2013SP2 on Windows - i.e liblzma, as it needs
C99.
We worked around it by including some logic to cmake, that if VC
compiler is less than specific version the support of dependent features
will be disabled.
https://github.com/nextgis-extra/lib_gdal
https://launchpad.net/~nextgis/+archive/ubuntu/ppa/+packages
Windows builds also using cmake: http://nextgis.ru/en/borsch/
About a month ago I made the hard decision to upgrade my entire MS4W
build environment from VC 2008 to 2015. Driving this is that PHP 7 only
supports 2015.
The only GDAL dependent library not supported for VC 2015 is FileGDB
(I've seen an Esri developer state that 2015 SDK will be released with
their next FileGDB API upgrade).
MrSID 9.5.1 SDK works well with 2015.
I didn't try an ECW 5.x SDK, but the old 3.3 SDK works well with 2015.
Short story is I support future GDAL builds requiring a more recent
Visual Studio compiler (2013+), as other projects are already doing so.
For those packagers cringing in the thought of upgrading their whole
stack: join the club :)
I should also note that for compiling Python 3.5.x VC 2015 is the
supported environment. Yet another GDAL dependent library that favors
an upgraded compiler on Windows.
Thanks for your feedback. Although I'd note that upgrading compiler version is
one thing, and enabling C++11 mode *might* be another, particularly if there's
a mix of C++ libs linking against each other. But from what I can read, it
seems that with Visual Studio you don't have the equivalent of -std=c++?? of
GCC, so if things are compiled with the same VS version, that should be OK.

Might be a little trickier with GCC, particularly with the new GCC 5 C++11
*ABI* and the dual ABI thing
(https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_dual_abi.html), but if
I understand well, if people use the same GCC version and do not play with
_GLIBCXX_USE_CXX11_ABI, linking code compiled with different -std=C++?? setting
should work.

I've confirmed it with the following experiment:

$ cat a.cpp
#include <string>

std::string foo()
{
return std::string("foo");
}

With gcc 5.2, both
g++ -std=c++11 -fPIC -shared a.cpp -o liba.so
g++ -std=c++03 -fPIC -shared a.cpp -o liba.so
result in the following symbol to be exported:
_Z3fooB5cxx11v

whereas if you add -D_GLIBCXX_USE_CXX11_ABI=0, the symbol name is _Z3foov,
which is the same as if you compile with GCC 4.X

So the potential problems would be when linking a GCC 5 compiled GDAL (whether
it be C++03 or C++11) against a GCC 4.X compiled lib (think of one of the
SDK), so this might already occur currently. Or reversely, if an
application/library compiled with GCC 4.X would link against GDAL compiled
with GCC 5.X

Even
--
Spatialys - Geospatial professional services
http://www.spatialys.com
Andrew Bell
2016-05-05 14:49:37 UTC
Permalink
Post by Even Rouault
Might be a little trickier with GCC, particularly with the new GCC 5 C++11
*ABI* and the dual ABI thing
(https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_dual_abi.html), but if
I understand well, if people use the same GCC version and do not play with
_GLIBCXX_USE_CXX11_ABI, linking code compiled with different -std=C++?? setting
should work.
$ cat a.cpp
#include <string>
std::string foo()
{
return std::string("foo");
}
With gcc 5.2, both
g++ -std=c++11 -fPIC -shared a.cpp -o liba.so
g++ -std=c++03 -fPIC -shared a.cpp -o liba.so
_Z3fooB5cxx11v
whereas if you add -D_GLIBCXX_USE_CXX11_ABI=0, the symbol name is _Z3foov,
which is the same as if you compile with GCC 4.X
So the potential problems would be when linking a GCC 5 compiled GDAL (whether
it be C++03 or C++11) against a GCC 4.X compiled lib (think of one of the
SDK), so this might already occur currently. Or reversely, if an
application/library compiled with GCC 4.X would link against GDAL compiled
with GCC 5.X
ABI compatibility is a mess: https://isocpp.org/files/papers/n4028.pdf

With that in mind, what you do inside your own codebase isn't much of an
issue and I'm guessing that even if GDAL wants to be as accommodating as
possible, maintainers of dependent libraries probably won't take such
care. In the end, if you're doing development and building things
yourself, you need to make sure you use the same compiler and switches for
everything if you're providing a C++ interface. Is this a pain? Yes. Is
it necessary? Yes.

As for the notion of replacing some allocations with vectors, well, great.
But the GDAL codebase is creaky in lots of places and to get too particular
about the advantage/disadvantage of vectors seems a little silly. At some
point it's just reasonable to say that you're only going to support
compilers/systems made after some date/version. People who don't want to
upgrade a system aren't harmed. They always have access to an older
version of the library. There is just so much cruft in GDAL that could
benefit from use of the standard library and C++ 11 that I don't know where
to start.

That said, changing code that works just because you like this or that
language feature always presents an opportunity for bugs to creep in.
--
Andrew Bell
***@gmail.com
Even Rouault
2016-05-05 15:15:38 UTC
Permalink
Post by Andrew Bell
Post by Even Rouault
Might be a little trickier with GCC, particularly with the new GCC 5
C++11 *ABI* and the dual ABI thing
(https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_dual_abi.html), but if
I understand well, if people use the same GCC version and do not play
with _GLIBCXX_USE_CXX11_ABI, linking code compiled with different
-std=C++?? setting
should work.
$ cat a.cpp
#include <string>
std::string foo()
{
return std::string("foo");
}
With gcc 5.2, both
g++ -std=c++11 -fPIC -shared a.cpp -o liba.so
g++ -std=c++03 -fPIC -shared a.cpp -o liba.so
_Z3fooB5cxx11v
whereas if you add -D_GLIBCXX_USE_CXX11_ABI=0, the symbol name is
_Z3foov, which is the same as if you compile with GCC 4.X
So the potential problems would be when linking a GCC 5 compiled GDAL (whether
it be C++03 or C++11) against a GCC 4.X compiled lib (think of one of the
SDK), so this might already occur currently. Or reversely, if an
application/library compiled with GCC 4.X would link against GDAL
compiled with GCC 5.X
ABI compatibility is a mess: https://isocpp.org/files/papers/n4028.pdf
With that in mind, what you do inside your own codebase isn't much of an
issue and I'm guessing that even if GDAL wants to be as accommodating as
possible, maintainers of dependent libraries probably won't take such
care. In the end, if you're doing development and building things
yourself, you need to make sure you use the same compiler and switches for
everything if you're providing a C++ interface. Is this a pain? Yes. Is
it necessary? Yes.
As for the notion of replacing some allocations with vectors, well, great.
But the GDAL codebase is creaky in lots of places and to get too particular
about the advantage/disadvantage of vectors seems a little silly. At some
point it's just reasonable to say that you're only going to support
compilers/systems made after some date/version. People who don't want to
upgrade a system aren't harmed. They always have access to an older
version of the library.
(Sometimes they want the latest version of the lib for a particular feature on a ancient system.)
Post by Andrew Bell
There is just so much cruft in GDAL that could
benefit from use of the standard library and C++ 11 that I don't know where
to start.
That would be interesting if you (or anyone) could list which C++11 features would be killer features
to justify the upgrade to C++11 vs the potential pains that such a move might cause (especially
as I raised in other emails, given the fact that GDAL uses a number of other libs and is used by a number
of other libs)
Post by Andrew Bell
That said, changing code that works just because you like this or that
language feature always presents an opportunity for bugs to creep in.
I agree completely with that :-) Especially for parts of the code base that are
not or poorly tested :
http://rawgit.com/rouault/gdalautotest-coverage-results/master/coverage_html/index.html
--
Spatialys - Geospatial professional services
http://www.spatialys.com
Kurt Schwehr
2016-05-05 15:19:48 UTC
Permalink
Thanks all for your comments! I will try to write up a similar proposal
for C++11 based on what people have written.

I've integrated (and still working on) a number of the points from the
discussion so far on the stack storage issue into the doc and added a
section titled "Impact of the proposed change"

This local stack storage change is for a real need, so it's probably good
that I add a use case that explains why this change is important. And I
believe<< (but am not 100% sure) that using std::vector is the strongest
solution for the majority of instances of large buffers on the stack in
gdal.

Use case: (note that I can only share generals)

At Google, we have jobs that use GDAL running with thousands of cores and
20 threads per process that run for days. The overhead of having to allow
for bumping the stack size adds substantial overhead to all those jobs.
GDAL is the only thing driving the requirement for the larger stack. For
people using GDAL on a desktop machine, the larger stack is unlikely to be
a real issue. For anyone running large multithreaded jobs in the cloud,
the large stack is a real cost issue. The overhead of vector and heap
allocation in all these cases I've looked at in GDAL so far is far smaller
than the cost from the stack size overhead.
Andrew Bell
2016-05-05 20:56:34 UTC
Permalink
Post by Kurt Schwehr
Thanks all for your comments! I will try to write up a similar proposal
for C++11 based on what people have written.
This local stack storage change is for a real need, so it's probably good
that I add a use case that explains why this change is important. And I
believe<< (but am not 100% sure) that using std::vector is the strongest
solution for the majority of instances of large buffers on the stack in
gdal.
Use case: (note that I can only share generals)
At Google, we have jobs that use GDAL running with thousands of cores and
20 threads per process that run for days. The overhead of having to allow
for bumping the stack size adds substantial overhead to all those jobs.
GDAL is the only thing driving the requirement for the larger stack. For
people using GDAL on a desktop machine, the larger stack is unlikely to be
a real issue. For anyone running large multithreaded jobs in the cloud,
the large stack is a real cost issue. The overhead of vector and heap
allocation in all these cases I've looked at in GDAL so far is far smaller
than the cost from the stack size overhead.
I misinterpreted this thread because of the title. It seems this isn't
really about code style or C++ 11. It's really about Google wanting to use
heap over stack because of its particular use of the library. There is no
need for C++ 11 for that and it would seem that doing a compile-time
policy-based array isn't too hard (proof of concept below). Perhaps Google
could flush something like this out to its liking and replace current stack
allocations that cause it issues. C++ 11 and code style seem another
conversation.

======

#include <iostream>

struct Stack
{};

struct Heap
{};

template <typename TYPE, std::size_t SIZE, typename AllocType>
struct Arr
{
};

template <typename TYPE, std::size_t SIZE>
struct Arr<TYPE, SIZE, Heap>
{
Arr() : m_instance(new TYPE[SIZE ? SIZE : 1])
{}

~Arr()
{
delete [] m_instance;
}

TYPE *m_instance;

TYPE& operator[](std::size_t idx)
{ return *(m_instance + idx); }
};

template <typename TYPE, std::size_t SIZE>
struct Arr<TYPE, SIZE, Stack>
{
TYPE m_instance[SIZE ? SIZE : 1];

TYPE& operator[](std::size_t idx)
{ return m_instance[idx]; }
};


#ifdef HEAPALLOC
template <typename T, std::size_t SIZE>
struct Array : public Arr<T, SIZE, Heap>
{};
#else
template <typename T, std::size_t SIZE>
struct Array : public Arr<T, SIZE, Stack>
{};
#endif

int main()
{
Array<int, 25> a;

a[10] = 15;
std::cout << "A[10] = " << a[10] << "!\n";

return 0;
}
--
Andrew Bell
***@gmail.com
Even Rouault
2016-05-05 21:25:11 UTC
Permalink
Post by Andrew Bell
I misinterpreted this thread because of the title. It seems this isn't
really about code style or C++ 11.
I agree this thread mixes different topics, which causes some confusion. The
word C++11 appearing in a doc has created some passion, whereas it is probably
not the heart of the subject.

Kurt has contributed to a lot of cleanup in the code base over the last
months, mostly in applying stricter formatting rules, bool'ifying,
const'ifying when possible, moving variable declarations close to their
initialization, etc.. (Kurt, correct me if I'm wrong.), also addressing a
bunch of Covertiy Scan warnings, etc.

I've actually encouraged him to share his thoughts regarding cleanups with the
rest of the community so that we can decide which are worth to be rules that
apply to the project as a whole, and if some tooling is needed to
automate/enforce them.
Post by Andrew Bell
It's really about Google wanting to use
heap over stack because of its particular use of the library. There is no
need for C++ 11 for that and it would seem that doing a compile-time
policy-based array isn't too hard (proof of concept below). Perhaps Google
could flush something like this out to its liking and replace current stack
allocations that cause it issues.
This use case is probably shared by other actors that use GDAL for massive
data processing, and if the solution to reduce stack usage doesn't hurt more
mundane use cases, we probably don't need the #ifdef complication.
--
Spatialys - Geospatial professional services
http://www.spatialys.com
Kurt Schwehr
2016-05-06 20:55:33 UTC
Permalink
I was not intending for C++11 to be a big part of the discussion as it is a
much more complicated topic, but I do appreciate the discussion. I picked
the stack + memset -> std::vector(nSize, initialValue) to do first because
I thought it was a simpler issue than most and we could use it to figure
out how to go about these sorts of discussions. It's not a show stoper,
but it is a real need.

At this point, I think it would be good if we could pause the C++11
discussion for a bit and back up to the original question.

I'd like to ask folks a couple of things:

- If you could stack rank (all or some) the options assuming that C++14 was
available
- Is your top pick far better than the rest or just a little better?
- Are there any options that you think should not be used in GDAL?
- Are there any reasons to be for or against any particular alternative
that we need to call out?

My belief is that for this particular proposal, we should not have the
C++11/14 discussion unless the best overall solution requires a newer
version of C++. The solution I propose to be the best works in C++03 and
newer.
Post by Even Rouault
Post by Andrew Bell
I misinterpreted this thread because of the title. It seems this isn't
really about code style or C++ 11.
I agree this thread mixes different topics, which causes some confusion. The
word C++11 appearing in a doc has created some passion, whereas it is probably
not the heart of the subject.
Kurt has contributed to a lot of cleanup in the code base over the last
months, mostly in applying stricter formatting rules, bool'ifying,
const'ifying when possible, moving variable declarations close to their
initialization, etc.. (Kurt, correct me if I'm wrong.), also addressing a
bunch of Covertiy Scan warnings, etc.
I've actually encouraged him to share his thoughts regarding cleanups with the
rest of the community so that we can decide which are worth to be rules that
apply to the project as a whole, and if some tooling is needed to
automate/enforce them.
Post by Andrew Bell
It's really about Google wanting to use
heap over stack because of its particular use of the library. There is
no
Post by Andrew Bell
need for C++ 11 for that and it would seem that doing a compile-time
policy-based array isn't too hard (proof of concept below). Perhaps
Google
Post by Andrew Bell
could flush something like this out to its liking and replace current
stack
Post by Andrew Bell
allocations that cause it issues.
This use case is probably shared by other actors that use GDAL for massive
data processing, and if the solution to reduce stack usage doesn't hurt more
mundane use cases, we probably don't need the #ifdef complication.
--
Spatialys - Geospatial professional services
http://www.spatialys.com
--
--
http://schwehr.org
Tim Keitt
2016-05-06 21:37:28 UTC
Permalink
http://www.keittlab.org/
Post by Kurt Schwehr
I was not intending for C++11 to be a big part of the discussion as it is
a much more complicated topic, but I do appreciate the discussion. I
picked the stack + memset -> std::vector(nSize, initialValue) to do first
because I thought it was a simpler issue than most and we could use it to
figure out how to go about these sorts of discussions. It's not a show
stoper, but it is a real need.
At this point, I think it would be good if we could pause the C++11
discussion for a bit and back up to the original question.
- If you could stack rank (all or some) the options assuming that C++14
was available
What are the options and for what purpose? (I have not followed the thread
in detail.) I'd like to see pointer types banished or if necessary wrapped
with std::auto_ptr (or whichever variant makes sense).
Post by Kurt Schwehr
- Is your top pick far better than the rest or just a little better?
- Are there any options that you think should not be used in GDAL?
- Are there any reasons to be for or against any particular alternative
that we need to call out?
Always prefer vanilla standard library over others.
Post by Kurt Schwehr
My belief is that for this particular proposal, we should not have the
C++11/14 discussion unless the best overall solution requires a newer
version of C++. The solution I propose to be the best works in C++03 and
newer.
I have longed for GDAL to be a modern C++ library that follows the STL, so
any move in that direction would be terrific in my opinion. It would be
nice to have the latest and greatest C++ bits, but I would be very happy
with C++03. I strongly suggest maximizing use of the STL to avoid
reinventing the wheel. If you need custom allocation, use a custom
allocator with an STL data structure. I would also recommend judicious use
of Boost where appropriate.

THK
Post by Kurt Schwehr
Post by Even Rouault
Post by Andrew Bell
I misinterpreted this thread because of the title. It seems this isn't
really about code style or C++ 11.
I agree this thread mixes different topics, which causes some confusion. The
word C++11 appearing in a doc has created some passion, whereas it is probably
not the heart of the subject.
Kurt has contributed to a lot of cleanup in the code base over the last
months, mostly in applying stricter formatting rules, bool'ifying,
const'ifying when possible, moving variable declarations close to their
initialization, etc.. (Kurt, correct me if I'm wrong.), also addressing a
bunch of Covertiy Scan warnings, etc.
I've actually encouraged him to share his thoughts regarding cleanups with the
rest of the community so that we can decide which are worth to be rules that
apply to the project as a whole, and if some tooling is needed to
automate/enforce them.
Post by Andrew Bell
It's really about Google wanting to use
heap over stack because of its particular use of the library. There is
no
Post by Andrew Bell
need for C++ 11 for that and it would seem that doing a compile-time
policy-based array isn't too hard (proof of concept below). Perhaps
Google
Post by Andrew Bell
could flush something like this out to its liking and replace current
stack
Post by Andrew Bell
allocations that cause it issues.
This use case is probably shared by other actors that use GDAL for massive
data processing, and if the solution to reduce stack usage doesn't hurt more
mundane use cases, we probably don't need the #ifdef complication.
--
Spatialys - Geospatial professional services
http://www.spatialys.com
--
--
http://schwehr.org
_______________________________________________
gdal-dev mailing list
http://lists.osgeo.org/mailman/listinfo/gdal-dev
Kurt Schwehr
2016-05-06 21:47:01 UTC
Permalink
For stack ranking, see the "alternatives" section of this (and I did not
include my preferred std::vector solution in the list):

https://docs.google.com/document/d/1Y9flzxj3Uz1vTEPCBmlswgi470m8i-oepGutVkbowfc/pub

BTW, Someone pointed out that I originally listed autoptr (which just won't
work) when I mean to say scoped_ptr.h. I chatted with the original author
of scoped_ptr last year and have permission to share the latest version
from inside Google should that somehow become the solution of choice for
any particular issue, but I really think we should avoid scoped_ptr when
unique_ptr exists, is a standard and is more robust. std::vector was not
the first solution I came up with for this issue.
Post by Tim Keitt
http://www.keittlab.org/
Post by Kurt Schwehr
I was not intending for C++11 to be a big part of the discussion as it is
a much more complicated topic, but I do appreciate the discussion. I
picked the stack + memset -> std::vector(nSize, initialValue) to do first
because I thought it was a simpler issue than most and we could use it to
figure out how to go about these sorts of discussions. It's not a show
stoper, but it is a real need.
At this point, I think it would be good if we could pause the C++11
discussion for a bit and back up to the original question.
- If you could stack rank (all or some) the options assuming that C++14
was available
What are the options and for what purpose? (I have not followed the thread
in detail.) I'd like to see pointer types banished or if necessary wrapped
with std::auto_ptr (or whichever variant makes sense).
Post by Kurt Schwehr
- Is your top pick far better than the rest or just a little better?
- Are there any options that you think should not be used in GDAL?
- Are there any reasons to be for or against any particular alternative
that we need to call out?
Always prefer vanilla standard library over others.
Post by Kurt Schwehr
My belief is that for this particular proposal, we should not have the
C++11/14 discussion unless the best overall solution requires a newer
version of C++. The solution I propose to be the best works in C++03 and
newer.
I have longed for GDAL to be a modern C++ library that follows the STL, so
any move in that direction would be terrific in my opinion. It would be
nice to have the latest and greatest C++ bits, but I would be very happy
with C++03. I strongly suggest maximizing use of the STL to avoid
reinventing the wheel. If you need custom allocation, use a custom
allocator with an STL data structure. I would also recommend judicious use
of Boost where appropriate.
THK
Post by Kurt Schwehr
Post by Even Rouault
Post by Andrew Bell
I misinterpreted this thread because of the title. It seems this isn't
really about code style or C++ 11.
I agree this thread mixes different topics, which causes some confusion. The
word C++11 appearing in a doc has created some passion, whereas it is probably
not the heart of the subject.
Kurt has contributed to a lot of cleanup in the code base over the last
months, mostly in applying stricter formatting rules, bool'ifying,
const'ifying when possible, moving variable declarations close to their
initialization, etc.. (Kurt, correct me if I'm wrong.), also addressing a
bunch of Covertiy Scan warnings, etc.
I've actually encouraged him to share his thoughts regarding cleanups with the
rest of the community so that we can decide which are worth to be rules that
apply to the project as a whole, and if some tooling is needed to
automate/enforce them.
Post by Andrew Bell
It's really about Google wanting to use
heap over stack because of its particular use of the library. There
is no
Post by Andrew Bell
need for C++ 11 for that and it would seem that doing a compile-time
policy-based array isn't too hard (proof of concept below). Perhaps
Google
Post by Andrew Bell
could flush something like this out to its liking and replace current
stack
Post by Andrew Bell
allocations that cause it issues.
This use case is probably shared by other actors that use GDAL for massive
data processing, and if the solution to reduce stack usage doesn't hurt more
mundane use cases, we probably don't need the #ifdef complication.
--
Spatialys - Geospatial professional services
http://www.spatialys.com
--
--
http://schwehr.org
_______________________________________________
gdal-dev mailing list
http://lists.osgeo.org/mailman/listinfo/gdal-dev
--
--
http://schwehr.org
Tim Keitt
2016-05-07 22:05:58 UTC
Permalink
Post by Kurt Schwehr
For stack ranking, see the "alternatives" section of this (and I did not
That seems like a reasonable solution. The function/method call interface
is where I would begin substituting STL for C equivalents if it were up to
me. Once those are set, then you can refactor as needed behind the API.

I was not recommending autoptr, I just did not go look up the replacements.
Also, much of C++11 can be accomplished using Boost as many of the features
originated there as libraries for C++03. A non-trivial dependency to be
sure.

THK

http://www.keittlab.org/
Andrew Bell
2016-05-06 21:47:19 UTC
Permalink
Post by Kurt Schwehr
I was not intending for C++11 to be a big part of the discussion as it is
a much more complicated topic, but I do appreciate the discussion. I
picked the stack + memset -> std::vector(nSize, initialValue) to do first
because I thought it was a simpler issue than most and we could use it to
figure out how to go about these sorts of discussions. It's not a show
stoper, but it is a real need.
At this point, I think it would be good if we could pause the C++11
discussion for a bit and back up to the original question.
- If you could stack rank (all or some) the options assuming that C++14
was available
- Is your top pick far better than the rest or just a little better?
- Are there any options that you think should not be used in GDAL?
- Are there any reasons to be for or against any particular alternative
that we need to call out?
Doing a heap-based array is simple. You can pretty-much copy the
std::array code from the standard library and introduce the indirection you
require. It's better than a vector for the reasons you already mentioned.
Something that looks and acts like an array is better than a vector because
it makes the use clear. If you have C++ 11 you can simply wrap
std::array. C++03 has value initialization for arrays: new int[10]();, so
that's another alternative.
--
Andrew Bell
***@gmail.com
Mateusz Loskot
2016-05-06 21:47:18 UTC
Permalink
Post by Kurt Schwehr
My belief is that for this particular proposal, we should not have the
C++11/14 discussion unless the best overall solution requires a newer
version of C++. The solution I propose to be the best works in C++03 and
newer.
Simply, the initial proposal was quite confusing.

The 'title' said indicated it was "mostly focused on C++11/14 & C99/11",
while the content mentioned mostly the std::vector as malloc'ed arrays
replacement.

Considering current codebase, GDAL is written in C and compiled in C++.
The only major C++ feature used in GDAL is classes and polymorphism.

I'm not sure if the facts mentioned in the proposal, namely
"Want to focus on maintainability and readability of code",
is meant to be a fact or a goal.
If you ask any seasoned C programmer for opinion, I bet she will
consider GDAL codebase as clear and readable.
If you ask a C++ programmer, the answer might be different.

IMO, I doubt any C++11/14 feature will increase the current code,
unless it is considered to ditch many of CPL features and replace
them with C++ standard features (string bashing operations, string containers,
containers for other types, threading API, etc.).

Certainly, any new line to be written may benefit from auto,
constexpr, using, nullptr, range loops - but for a "C with classes"
codebase as GDAL, it would be cosmetic improvement, if any.

Surely, the override and final will help compiler to catch potential bugs.
Also, C++11/14 compilation mode is generally stricter, so it might
help to catch bugs.

Simply, the whole proposal is becoming vague.

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
Tim Keitt
2016-05-07 21:57:20 UTC
Permalink
http://www.keittlab.org/
Post by Mateusz Loskot
Post by Kurt Schwehr
My belief is that for this particular proposal, we should not have the
C++11/14 discussion unless the best overall solution requires a newer
version of C++. The solution I propose to be the best works in C++03 and
newer.
Simply, the initial proposal was quite confusing.
The 'title' said indicated it was "mostly focused on C++11/14 & C99/11",
while the content mentioned mostly the std::vector as malloc'ed arrays
replacement.
Considering current codebase, GDAL is written in C and compiled in C++.
The only major C++ feature used in GDAL is classes and polymorphism.
I'm not sure if the facts mentioned in the proposal, namely
"Want to focus on maintainability and readability of code",
is meant to be a fact or a goal.
If you ask any seasoned C programmer for opinion, I bet she will
consider GDAL codebase as clear and readable.
If you ask a C++ programmer, the answer might be different.
IMO, I doubt any C++11/14 feature will increase the current code,
unless it is considered to ditch many of CPL features and replace
them with C++ standard features (string bashing operations, string containers,
containers for other types, threading API, etc.).
That's exactly my preference. For those of us who have to remember too many
things, using the standard library is a godsend because it is exceptionally
well documented online and used in many places.
Post by Mateusz Loskot
Certainly, any new line to be written may benefit from auto,
constexpr, using, nullptr, range loops - but for a "C with classes"
codebase as GDAL, it would be cosmetic improvement, if any.
It is true that unless you refactor along the lines of Boost, then these
features are not used as effectively.
Post by Mateusz Loskot
Surely, the override and final will help compiler to catch potential bugs.
Also, C++11/14 compilation mode is generally stricter, so it might
help to catch bugs.
My experience with C++11 is that it is vastly less error prone and more
readable. But of course there is a big cost to rewriting code, so I
understand the conundrum.

THK
Post by Mateusz Loskot
Simply, the whole proposal is becoming vague.
Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
_______________________________________________
gdal-dev mailing list
http://lists.osgeo.org/mailman/listinfo/gdal-dev
Kurt Schwehr
2016-05-08 06:01:09 UTC
Permalink
Mateusz,

Thanks for calling out the confusion! Are meaning to say that my proposal
<https://docs.google.com/document/d/1Y9flzxj3Uz1vTEPCBmlswgi470m8i-oepGutVkbowfc/pub>
is
vague? I rephrased the status to be more clear that the C++11/14 focus is
the GDAL community (AKA the majority of this thread), not the proposal.
Hopefully it is clearer now. Please read the proposal as just looking at
the possibilities. Please try not to care about C++ language version when
trying to do a first round review of ideas.
Post by Mateusz Loskot
Post by Kurt Schwehr
My belief is that for this particular proposal, we should not have the
C++11/14 discussion unless the best overall solution requires a newer
version of C++. The solution I propose to be the best works in C++03 and
newer.
Simply, the initial proposal was quite confusing.
The 'title' said indicated it was "mostly focused on C++11/14 & C99/11",
while the content mentioned mostly the std::vector as malloc'ed arrays
replacement.
That was just a status I added after the fact. I worded it poorly.
Post by Mateusz Loskot
Considering current codebase, GDAL is written in C and compiled in C++.
The only major C++ feature used in GDAL is classes and polymorphism.
I agree that GDAL has a strong feel of C compiled with C++ right now.

I disagree about C++ feature use. I see algorithm, iostream, map, list,
sstream, a few exceptions, std::string (mostly in the form of CPLString),
vector, localization of variables, some templates, #define to static
consts, etc.

I'm not sure if the facts mentioned in the proposal, namely
Post by Mateusz Loskot
"Want to focus on maintainability and readability of code",
is meant to be a fact or a goal.
That would be a goal. And readability is going somewhat be subjective.
Post by Mateusz Loskot
If you ask any seasoned C programmer for opinion, I bet she will
consider GDAL codebase as clear and readable.
If you ask a C++ programmer, the answer might be different.
IMO, I doubt any C++11/14 feature will increase the current code,
unless it is considered to ditch many of CPL features and replace
them with C++ standard features (string bashing operations, string containers,
containers for other types, threading API, etc.).
"increase the current code" <- Did you mean increase the reliability? Or
size? Or complexity?
Post by Mateusz Loskot
Certainly, any new line to be written may benefit from auto,
constexpr, using, nullptr, range loops - but for a "C with classes"
codebase as GDAL, it would be cosmetic improvement, if any.
I disagree that C++11/14 changes would be cosmetic. But this is the wrong
thread for this part of the discussion.
Post by Mateusz Loskot
Surely, the override and final will help compiler to catch potential bugs.
Also, C++11/14 compilation mode is generally stricter, so it might
help to catch bugs.
Simply, the whole proposal is becoming vague.
Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
Mateusz Loskot
2016-05-09 12:59:47 UTC
Permalink
Are meaning to say that my proposal is vague?
Yes, I called that on the course of the discussion, it has become less and less
clear to me: what is the actual goal?
- make the stack/heap usage more configurable
- use new C++ features to introduce widely recommended idioms
- make the code shorter, hence clearer/more readable/maintainable
- make the code more C++ than C by allowing wide use of C++ standard features
...
So, in order to achive the above, you are proposing switch to newer C++ version?
Please read the proposal as just looking at the possibilities.
Please try not to care about C++ language version when
trying to do a first round review of ideas.
I have and as far as I can see, there are just two objectives:
1. Make GDAL stack-friendly by preferring heap for arrays.
How? Use standard containers and smart pointers.
2. Introduce RAII across the whole codebase.
How? Use smart pointers and other related techniques.

That's all I can see from most of the content there.
(The many fine-grained details are not critical to make the actual
'big' decisions, those will come into play later.)

So, as I have already mentioned, great.
Now, let's decide wheter we switch to C++11 to get the necessary tools or
implement our own tools, e.g. as part of CPL.

I believe, much of the discussion would have gained more focus if the problem
was stated like that.
Post by Mateusz Loskot
Post by Kurt Schwehr
My belief is that for this particular proposal, we should not have the
C++11/14 discussion unless the best overall solution requires a newer
version of C++. The solution I propose to be the best works in C++03
and newer.
Considering current codebase, GDAL is written in C and compiled in C++.
The only major C++ feature used in GDAL is classes and polymorphism.
I agree that GDAL has a strong feel of C compiled with C++ right now.
Since that is a fact, there is nothing to agree about.
Please, notice, I'm not speaking here about my personal preferences,
just the state of the art.
I disagree about C++ feature use. I see algorithm, iostream, map, list,
sstream, a few exceptions, std::string (mostly in the form of CPLString),
vector, localization of variables, some templates, #define to static consts,
etc.
In GDAL? e.g. streams are not used or they use is marginal.
Post by Mateusz Loskot
If you ask any seasoned C programmer for opinion, I bet she will
consider GDAL codebase as clear and readable.
If you ask a C++ programmer, the answer might be different.
IMO, I doubt any C++11/14 feature will increase the current code,
unless it is considered to ditch many of CPL features and replace
them with C++ standard features (string bashing operations, string containers,
containers for other types, threading API, etc.).
"increase the current code" <- Did you mean increase the reliability? Or
size? Or complexity?
Sorry, sloppy typing. I meant code clarity, readability.
Post by Mateusz Loskot
Certainly, any new line to be written may benefit from auto,
constexpr, using, nullptr, range loops - but for a "C with classes"
codebase as GDAL, it would be cosmetic improvement, if any.
I disagree that C++11/14 changes would be cosmetic. But this is the wrong
thread for this part of the discussion.
IMHO, you are missing one important point.
I'm not arguing that shifting from C with classes mode to C++11-17
is not going to increase software quality. Of course it is, if applied
properly and *at all*.

Have you considered current maintainers might not be willing to start using
the latest and the greatest C++11-17 features?
If they are not...changes will be rather cosmectic.

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
Kurt Schwehr
2016-05-09 14:47:26 UTC
Permalink
I was trying to give people an overall sense of my goals for GDAL.

I'm sorry I added the context to the proposal that got the discussion off
the track I intended. I am more interested in mechanics of the discussion
before hitting huge topics. I love the discussion, but I feel like I
totally failed at trying to test strategy for the GDAL community to have
discussions and get some basics added to the style guide with some why
attached. I think we have lots to discuss before tooling and newer
versions of C++. There are lots of implicit style things in GDAL that I
have been trying to bend my coding to, but I find clashing styles in the
same file all the time, so it is hard to get what I do to match.

Just for this proposal... I do let myself get dragged a bit into the C++
version discussion a bit below to try to show that it is off topic of
Post by Mateusz Loskot
getting large objects off the stack<<<.
Are meaning to say that my proposal is vague?
Yes, I called that on the course of the discussion, it has become less and less
clear to me: what is the actual goal?
- make the stack/heap usage more configurable
No. Not configurable. I propose to get large objects off the stack unless
a particular case severely impacts performance.
- use new C++ features to introduce widely recommended idioms
No. I do not propose using any C++ features new to GDAL. This is just the
old vector size with initializer constructor.
Post by Mateusz Loskot
- make the code shorter, hence clearer/more readable/maintainable
No. Shorter wasn't a goal for this one. It's sorter because my proposed
solution combines the definition and the initialization so as to not have
an uninitialized variable after the definition until the memset.

- make the code more C++ than C by allowing wide use of C++ standard
Post by Mateusz Loskot
features
No.
Post by Mateusz Loskot
...
So, in order to achive the above, you are proposing switch to newer C++ version?
No. I was trying to be complete in going through all the options available
inside and out of GDAL. Only if the community really wanted a feature from
C++ >= 11 would I consider the language version a part of this proposal.
Post by Mateusz Loskot
Please read the proposal as just looking at the possibilities.
Please try not to care about C++ language version when
trying to do a first round review of ideas.
1. Make GDAL stack-friendly by preferring heap for arrays.
How? Use standard containers and smart pointers.
That is 3 statements:

a) Make GDAL stack-friendly by preferring heap for arrays.

Yes.

b) How? Use standard containers

Yes. I propose this

c) and smart pointers

No. In this case, a smart pointer looks to be less attractive (to me).
Post by Mateusz Loskot
2. Introduce RAII across the whole codebase.
How? Use smart pointers and other related techniques.
No! Out of scope of this proposal. (For another proposal, then yes).
Post by Mateusz Loskot
That's all I can see from most of the content there.
(The many fine-grained details are not critical to make the actual
'big' decisions, those will come into play later.)
So, as I have already mentioned, great.
Now, let's decide wheter we switch to C++11 to get the necessary tools or
implement our own tools, e.g. as part of CPL.
No. Please do not do that with my proposal. That is way way too far out
of scope of what I intended for *this* proposal. Can we start with just
this simple proposal?
Post by Mateusz Loskot
I believe, much of the discussion would have gained more focus if the problem
was stated like that.
Yeah, I missed the boat on focus.
Post by Mateusz Loskot
Post by Kurt Schwehr
My belief is that for this particular proposal, we should not have the
C++11/14 discussion unless the best overall solution requires a newer
version of C++. The solution I propose to be the best works in C++03
and newer.
Considering current codebase, GDAL is written in C and compiled in C++.
The only major C++ feature used in GDAL is classes and polymorphism.
I agree that GDAL has a strong feel of C compiled with C++ right now.
Since that is a fact, there is nothing to agree about.
Please, notice, I'm not speaking here about my personal preferences,
just the state of the art.
I disagree about C++ feature use. I see algorithm, iostream, map, list,
sstream, a few exceptions, std::string (mostly in the form of CPLString),
vector, localization of variables, some templates, #define to static
consts,
etc.
In GDAL? e.g. streams are not used or they use is marginal.
find . -name \*cpp | xargs grep include | grep stream | wc -l
53

find . -name \*cpp | xargs grep -h include | grep stream | sort -u
#include <fstream>
#include <iostream>
#include "mrsidstream.h"
//#include <sstream>
#include <sstream>
#include <strstream>
Post by Mateusz Loskot
If you ask any seasoned C programmer for opinion, I bet she will
consider GDAL codebase as clear and readable.
If you ask a C++ programmer, the answer might be different.
IMO, I doubt any C++11/14 feature will increase the current code,
unless it is considered to ditch many of CPL features and replace
them with C++ standard features (string bashing operations, string containers,
containers for other types, threading API, etc.).
"increase the current code" <- Did you mean increase the reliability? Or
size? Or complexity?
Sorry, sloppy typing. I meant code clarity, readability.
Certainly, any new line to be written may benefit from auto,
constexpr, using, nullptr, range loops - but for a "C with classes"
codebase as GDAL, it would be cosmetic improvement, if any.
I disagree that C++11/14 changes would be cosmetic. But this is the
wrong
thread for this part of the discussion.
IMHO, you are missing one important point.
I'm not arguing that shifting from C with classes mode to C++11-17
is not going to increase software quality. Of course it is, if applied
properly and *at all*.
Have you considered current maintainers might not be willing to start using
the latest and the greatest C++11-17 features?
If they are not...changes will be rather cosmectic.
Yes, I understand this. If you look at this proposal, you will find that I
was not proposing >= C++11. I am working on a proposal for newer
versions. I am thinking gradually. There are non-cosmetic things that are
not very different but just work way better in the new standards. And if
we go through a proposal process, you can have/should demand clean
examples. And I think we should have a vote on these features. If
maintainers are not ready for a feature, then vote against it! Try to
think of baby steps. If we say yes to C++11 being required, I think my
first proposal would be to switch NULL to nullptr (which is typed). My
experience is that it does catch a number of strange edge case issues.
They are rare, but I have hit them. While GDAL isn't huge, GDAL is big
enough that NULL might be be hiding these issues or it might happen in
future commits. I don't think changing NULL to nullptr shouldn't be too
scary to C coders.

Even just switching the C++ version to C++11 would improve checking w.r.t.
the std lib, reduce the testing load on travis-ci, let us drop a couple
macros, and slightly simplify the build system.

But, all that is out of scope of my first proposal.
Post by Mateusz Loskot
Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
--
--
http://schwehr.org
Mateusz Loskot
2016-05-09 16:21:02 UTC
Permalink
There are lots of implicit style things in GDAL that I have been
trying to bend my coding to, but I find clashing styles in the same file all
the time, so it is hard to get what I do to match.
Just choose the one of the two you like more.
Post by Mateusz Loskot
- make the stack/heap usage more configurable
No. Not configurable. I propose to get large objects off the stack unless
a particular case severely impacts performance.
Yes, I got it,
I meant coding/compile-time configurable, if necessary, with
standard container/custom allocator/traits, etc.
There could even be a #define to control that all current T buffer[12345]
become heap-based or stay stack-based buffers.
However, actual realisation does not matter at this point.
Post by Mateusz Loskot
- use new C++ features to introduce widely recommended idioms
No. I do not propose using any C++ features new to GDAL. This is just the
old vector size with initializer constructor.
As one of your argument, I have seen RAII proposed.
Post by Mateusz Loskot
Please read the proposal as just looking at the possibilities.
Please try not to care about C++ language version when
trying to do a first round review of ideas.
1. Make GDAL stack-friendly by preferring heap for arrays.
How? Use standard containers and smart pointers.
a) Make GDAL stack-friendly by preferring heap for arrays.
Yes.
OK,
b) How? Use standard containers
Yes. I propose this
OK
c) and smart pointers
No. In this case, a smart pointer looks to be less attractive (to me).
Post by Mateusz Loskot
2. Introduce RAII across the whole codebase.
How? Use smart pointers and other related techniques.
No! Out of scope of this proposal. (For another proposal, then yes).
Again, I have seen arguments like this
https://lists.osgeo.org/pipermail/gdal-dev/2016-May/044355.html

But, OK, I assume it is no longer part of your proposal.
Post by Mateusz Loskot
That's all I can see from most of the content there.
(The many fine-grained details are not critical to make the actual
'big' decisions, those will come into play later.)
So, as I have already mentioned, great.
Now, let's decide wheter we switch to C++11 to get the necessary tools or
implement our own tools, e.g. as part of CPL.
No. Please do not do that with my proposal. That is way way too far out of
scope of what I intended for *this* proposal.
I'm not doing anything with it.
I'm just asking questions and stating my understanding of your goal,
some I might have got wrong.
Can we start with just this simple proposal?
Sure. I have got my issues cleared, thanks.

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
Kurt Schwehr
2016-05-09 17:02:21 UTC
Permalink
All,

Just to be totally obvious and not meant to squash questions, comments,
concerns. I'm trying to narrow the scope of this discussion. <y choice of
title for this thread on the mailing list is not good. Anyone object to me
starting a different subject line to continue the conversation on this
large object on the stack proposal?

Mateusz,

Sounds like we are on the right track now. Hopefully, the proposal is
clearer after a few more changes just now.

My concern is that you seem to taking the discuss as the proposal and not
ready the proposal as stand alone: http://goo.gl/vuA3D6 (Especially after
I fixed the "Status:"... which I just changed again). My intent was to
have a super narrow focus to the proposal, but be complete thereby letting
the reader know that I'm aware of alternatives. I thought that this change
would make the scope crystal clear (obvious I was wrong :)...

https://trac.osgeo.org/gdal/changeset/34177

unique_ptr was in there as an alternative. The item you refer to was the
Post by Mateusz Loskot
Post by Kurt Schwehr
side track<<< of C++11 in the mailing list. It belongs as a separate
proposal dependent on a C++11 proposal. And on the side track of C++11
support: I think your concerns are very valid. Let's keep those to the C++
language version thread.

Anything in this thread is a great discussion, but is NOT the proposal.
That document never said that it is a justification in any way for C++11.

I added a note at the top to clarify. The what if section is just food for
thought / completeness. I regret having it there in the beginning, but I
might as well leave it now that the confusion already happened.

I'm happy to clarify the text in the proposal or give you comment/suggest
access to the doc.
Post by Mateusz Loskot
Post by Kurt Schwehr
No! Out of scope of this proposal. (For another proposal, then yes).
Again, I have seen arguments like this
https://lists.osgeo.org/pipermail/gdal-dev/2016-May/044355.html
But, OK, I assume it is no longer part of your proposal.
Never was part of the proposed solution for "Use vector<T>(length,
initial_value) for local blocks of storage."
Post by Mateusz Loskot
Post by Kurt Schwehr
That's all I can see from most of the content there.
(The many fine-grained details are not critical to make the actual
'big' decisions, those will come into play later.)
So, as I have already mentioned, great.
Now, let's decide wheter we switch to C++11 to get the necessary tools
or
Post by Kurt Schwehr
implement our own tools, e.g. as part of CPL.
No. Please do not do that with my proposal. That is way way too far
out of
Post by Kurt Schwehr
scope of what I intended for *this* proposal.
I'm not doing anything with it.
I'm just asking questions and stating my understanding of your goal,
some I might have got wrong.
Please just take the text in the doc as the proposal. Unless I copy text
from the mailing list to the doc, I don't consider it a part of the
proposal.
Post by Mateusz Loskot
Post by Kurt Schwehr
Can we start with just this simple proposal?
Sure. I have got my issues cleared, thanks.
Great! Thanks for helping me improve the doc.
Post by Mateusz Loskot
Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
Even Rouault
2016-05-09 17:29:42 UTC
Permalink
Post by Kurt Schwehr
http://goo.gl/vuA3D6 (Especially after
I fixed the "Status:"... which I just changed again).
Looks good to me.

Perhaps 256 bytes as the threshold ? (a LUT of 256 byte elements would fit) By
the way, is it a threshold per array or for all arrays of a given function ?

I can think of some *potentially* performance sensitive locations like
RPCTransformPoint() that uses a double [20+1] array where, if it would be
above the threshold, moving the array to a working structure
(GDALRPCTransformInfo* in that instance), or to the upper layers if they call
multiple times the subroutine, could both address the stack size and
performance. But at that point it is hard to be able to predict that, so let's
do no premature optimization.
--
Spatialys - Geospatial professional services
http://www.spatialys.com
Mateusz Loskot
2016-05-09 19:49:52 UTC
Permalink
Post by Kurt Schwehr
Sounds like we are on the right track now. Hopefully, the proposal is
clearer after a few more changes just now.
My concern is that you seem to taking the discuss as the proposal and not
ready the proposal as stand alone: http://goo.gl/vuA3D6
Kurt,

Point taken.

Although the proposal looks OK, I'd suggest to check what
assembler code generates your favourite C++ toolkit,
or at least measured times for

int anVals[256];
memset(anVals, 0, 256*sizeof(int));

vs

std::vector<int> oVals(256, 0);

and compare with:

std::vector<char> oVals(256, 0);

It is an interesting experiment.

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
Andrew Bell
2016-05-09 20:37:20 UTC
Permalink
Post by Mateusz Loskot
Point taken.
Although the proposal looks OK, I'd suggest to check what
assembler code generates your favourite C++ toolkit,
or at least measured times for
int anVals[256];
memset(anVals, 0, 256*sizeof(int));
Are "we" doing memset anymore in these cases?

int anVals[256] = {};

seems preferable
Post by Mateusz Loskot
vs
std::vector<int> oVals(256, 0);
std::vector<char> oVals(256, 0);
Think vector is a bad solution for something that's fixed. Just write
something. But I already suggested that and wrote something... :)

Do you know why they are wedded to a 16K stack?
--
Andrew Bell
***@gmail.com
Kurt Schwehr
2016-06-01 22:46:06 UTC
Permalink
https://docs.google.com/document/d/1O1B7LY13L532kXcYcB2EdO65m5LOCsqaqn5R9iJfSPU/pub

The optimized stack .o is 1248 bytes and the on the heap vector is 1600
bytes with gcc 4.8. The cost of either is pretty small. So, if there are
100 of these in gdal, we are talking about 30-60K of extra object file size
for all the cases in GDAL.

For stack usage, start thinking about tons of threads spread out over lots
of cores and think about how systems are built... Google Compute Engine
has 32 core machines. If you want to get the most out your machines at
scale, you want a small stack.

int anVals[256] = {}; Does initialize everything to the default value
(zeros), but doesn't solve the stack issue.

Making a little class is yet another thing for people to learn when vector
looks to me to work quite well.
Post by Andrew Bell
Post by Mateusz Loskot
Point taken.
Although the proposal looks OK, I'd suggest to check what
assembler code generates your favourite C++ toolkit,
or at least measured times for
int anVals[256];
memset(anVals, 0, 256*sizeof(int));
Are "we" doing memset anymore in these cases?
int anVals[256] = {};
seems preferable
Post by Mateusz Loskot
vs
std::vector<int> oVals(256, 0);
std::vector<char> oVals(256, 0);
Think vector is a bad solution for something that's fixed. Just write
something. But I already suggested that and wrote something... :)
Do you know why they are wedded to a 16K stack?
--
Andrew Bell
_______________________________________________
gdal-dev mailing list
http://lists.osgeo.org/mailman/listinfo/gdal-dev
--
--
http://schwehr.org
Even Rouault
2016-06-02 10:29:37 UTC
Permalink
Post by Kurt Schwehr
https://docs.google.com/document/d/1O1B7LY13L532kXcYcB2EdO65m5LOCsqaqn5R9iJ
fSPU/pub
The optimized stack .o is 1248 bytes and the on the heap vector is 1600
bytes with gcc 4.8. The cost of either is pretty small. So, if there are
100 of these in gdal, we are talking about 30-60K of extra object file size
for all the cases in GDAL.
Code size is one thing, but I'd be curious to know about the speed impacts due
to the heap allocation. Like time several million iterations of each way (and
make sure to do something non trivial with the arrray so that the compiler
doesn't optimize the code away). If there's no significant difference, then fine.
Otherwise we'd have to be careful when the replacements are done in a
performance sensitive routine (my feeling is that there are not so many such
places)
Post by Kurt Schwehr
For stack usage, start thinking about tons of threads spread out over lots
of cores and think about how systems are built... Google Compute Engine
has 32 core machines. If you want to get the most out your machines at
scale, you want a small stack.
int anVals[256] = {}; Does initialize everything to the default value
(zeros), but doesn't solve the stack issue.
Making a little class is yet another thing for people to learn when vector
looks to me to work quite well.
Post by Andrew Bell
Post by Mateusz Loskot
Point taken.
Although the proposal looks OK, I'd suggest to check what
assembler code generates your favourite C++ toolkit,
or at least measured times for
int anVals[256];
memset(anVals, 0, 256*sizeof(int));
Are "we" doing memset anymore in these cases?
int anVals[256] = {};
seems preferable
Post by Mateusz Loskot
vs
std::vector<int> oVals(256, 0);
std::vector<char> oVals(256, 0);
Think vector is a bad solution for something that's fixed. Just write
something. But I already suggested that and wrote something... :)
Do you know why they are wedded to a 16K stack?
--
Andrew Bell
_______________________________________________
gdal-dev mailing list
http://lists.osgeo.org/mailman/listinfo/gdal-dev
--
Spatialys - Geospatial professional services
http://www.spatialys.com
Kurt Schwehr
2016-06-03 13:54:24 UTC
Permalink
I would make some of assertions (some just restating what Even wrote)

* The cost to heap allocation is a fair question (but I would suggest we
defer the work until required)

* Any work done on performance critical subroutines should be done
carefully.

* It would help to have a standard comment that people can add to
empirically detected performance critical sections so that we reduce the
chance of performance regressions

* It would be nice to have a very simple performance check system, but that
seems like a separate discussion that will open the whole benchmarking
can-o-worms.

* It would be great to have a set of instructions on how to instrument GDAL
to collect metrics of runs and detect hot spots with open source tools.
It's been a long time since I've done anything like that with generally
available tools. e.g. https://sourceware.org/binutils/docs/gprof/ And I
need to look more at the gcov output. I see that mapserver has a start
https://trac.osgeo.org/mapserver/wiki/PerformanceTesting

* The gdal wiki needs to have more of
http://erouault.blogspot.com/2016/01/software-quality-improvements-in-gdal.html
blended in

* https://en.wikiquote.org/wiki/Donald_Knuth "premature optimization is the
root of all evil (or at least most of it) in programming" :)

* I would suggest that with any guideline there will almost always be the
occasional location where that guideline does not make sense. In that
case, I would expect that a comment would go along with the exception
explaining why it is important.

* The cost of heap versus stack is not well done in a standalone test (but
there is value in standalone examples). Pressures on caches, TLBs, etc are
difficult to replicate in simple examples. Moving a large allocate to the
heap helps keep the rest of the stack fast. Think about the pages required
for the stack with multiple cores and lots of threads.
http://unix.stackexchange.com/questions/128213/how-is-page-size-determined-in-virtual-address-space

* There are also places where heap allocation is just flat out banned after
a process is initialized. e.g. daemons like gpsd or long running realtime
systems (your car's engine controllers, spacecraft control systems, etc.).
I would argue that it is never going to be a good idea to use GDAL inside
that kind of system / process

In terms of GDAL, I haven't seen any places where heap allocation of large
objects would like be an important fraction of runtime. I'm sure they are
there, but I would bet they are rare.
Post by Kurt Schwehr
https://docs.google.com/document/d/1O1B7LY13L532kXcYcB2EdO65m5LOCsqaqn5R9iJ
Post by Kurt Schwehr
fSPU/pub
The optimized stack .o is 1248 bytes and the on the heap vector is 1600
bytes with gcc 4.8. The cost of either is pretty small. So, if there
are
Post by Kurt Schwehr
100 of these in gdal, we are talking about 30-60K of extra object file
size
Post by Kurt Schwehr
for all the cases in GDAL.
Code size is one thing, but I'd be curious to know about the speed impacts due
to the heap allocation. Like time several million iterations of each way (and
make sure to do something non trivial with the arrray so that the compiler
doesn't optimize the code away). If there's no significant difference, then fine.
Otherwise we'd have to be careful when the replacements are done in a
performance sensitive routine (my feeling is that there are not so many such
places)
Post by Kurt Schwehr
For stack usage, start thinking about tons of threads spread out over
lots
Post by Kurt Schwehr
of cores and think about how systems are built... Google Compute Engine
has 32 core machines. If you want to get the most out your machines at
scale, you want a small stack.
int anVals[256] = {}; Does initialize everything to the default value
(zeros), but doesn't solve the stack issue.
Making a little class is yet another thing for people to learn when
vector
Post by Kurt Schwehr
looks to me to work quite well.
Post by Andrew Bell
Post by Mateusz Loskot
Point taken.
Although the proposal looks OK, I'd suggest to check what
assembler code generates your favourite C++ toolkit,
or at least measured times for
int anVals[256];
memset(anVals, 0, 256*sizeof(int));
Are "we" doing memset anymore in these cases?
int anVals[256] = {};
seems preferable
Post by Mateusz Loskot
vs
std::vector<int> oVals(256, 0);
std::vector<char> oVals(256, 0);
Think vector is a bad solution for something that's fixed. Just write
something. But I already suggested that and wrote something... :)
Do you know why they are wedded to a 16K stack?
--
Andrew Bell
_______________________________________________
gdal-dev mailing list
http://lists.osgeo.org/mailman/listinfo/gdal-dev
--
Spatialys - Geospatial professional services
http://www.spatialys.com
--
--
http://schwehr.org
Mateusz Loskot
2016-06-03 14:31:50 UTC
Permalink
Post by Kurt Schwehr
I would make some of assertions (some just restating what Even wrote)
* The cost to heap allocation is a fair question (but I would suggest we
defer the work until required)
* Any work done on performance critical subroutines should be done
carefully.
I would precise:

Any major changes that are supposed to improve performance
should be first backed up by profiling results or complexity analysis
that clearly confirm promised gains.

Such results also could be used as not-slower-than reference
in later benchmarks.

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
Kurt Schwehr
2016-05-09 17:11:02 UTC
Permalink
I intend to argue strongly against the many styles at a later date and in a
separate thread.
Post by Mateusz Loskot
There are lots of implicit style things in GDAL that I have been
trying to bend my coding to, but I find clashing styles in the same file
all
the time, so it is hard to get what I do to match.
Just choose the one of the two you like more.
Andrew Bell
2016-05-05 15:21:04 UTC
Permalink
Post by Andrew Bell
Post by Andrew Bell
As for the notion of replacing some allocations with vectors, well,
great.
Post by Andrew Bell
But the GDAL codebase is creaky in lots of places and to get too
particular
Post by Andrew Bell
about the advantage/disadvantage of vectors seems a little silly. At
some
Post by Andrew Bell
point it's just reasonable to say that you're only going to support
compilers/systems made after some date/version. People who don't want to
upgrade a system aren't harmed. They always have access to an older
version of the library.
(Sometimes they want the latest version of the lib for a particular
feature on a ancient system.)
Yes, but I'm unsympathetic. This is free software. To make the system
less maintainable for developers often working for free and more difficult
for many other users for the sake of a few outliers is to me unappealing.
If people want a feature integrated into some old system, they can
certainly hire someone to do just that.
--
Andrew Bell
***@gmail.com
Mark Coletti
2016-05-05 19:43:34 UTC
Permalink
Post by Even Rouault
[...]
That would be interesting if you (or anyone) could list which C++11
features would be killer features
to justify the upgrade to C++11 vs the potential pains that such a move
might cause (especially
as I raised in other emails, given the fact that GDAL uses a number of
other libs and is used by a number
of other libs) [...]
This is an off the cuff list of C++-11 niceties, and by no means exhaustive:

- std::move()
- *lambda expressions*
- auto keyword
- native smart pointer support, such as std::unique_ptr and std::shared_ptr
(though there's always the Boost fallback)
- concurrency API
- nullptr


Also, it's been many, many years since I last looked at GDAL/OGR source.
When I did I noted a lot of C-style I/O calls instead of using C++
equivalents. Is that still the case? If so, that's one area ripe for
refactoring.

Cheers,

Mark
--
***@gmail.com
Ray Gardener
2016-05-05 20:04:16 UTC
Permalink
How about C++11 threads?

Ray
On Thu, May 5, 2016 at 11:15 AM, Even Rouault
[...]
That would be interesting if you (or anyone) could list which
C++11 features would be killer features
to justify the upgrade to C++11 vs the potential pains that such a
move might cause (especially
as I raised in other emails, given the fact that GDAL uses a
number of other libs and is used by a number
of other libs) [...]
- std::move()
- *lambda expressions*
- auto keyword
- native smart pointer support, such as std::unique_ptr and
std::shared_ptr (though there's always the Boost fallback)
- concurrency API
- nullptr
Also, it's been many, many years since I last looked at GDAL/OGR
source. When I did I noted a lot of C-style I/O calls instead of
using C++ equivalents. Is that still the case? If so, that's one
area ripe for refactoring.
Cheers,
Mark
--
_______________________________________________
gdal-dev mailing list
http://lists.osgeo.org/mailman/listinfo/gdal-dev
Joaquim Luis
2016-05-05 12:37:19 UTC
Permalink
I'm also updating my builds to support 2015 for a future inclusion in GMT.
There is, however, a bit storm ahead. Files created with HDF5.10 break
compatibility with older versions and can't be read. Put netCDF in this
bag as well. See GMT's #3495 for a recent case.

http://gmt.soest.hawaii.edu/boards/1/topics/3495
Post by Jeff McKenna
Post by Dmitry Baryshnikov
Hi,
I think it's time to go forward and shift to C++11 in i.e. GDAL 2.2 and
drop old staff as it was with Windows mobile support, VB, etc.
Yes, it may be some regression in ABI, but this is less evil than
support lot of ancient compilers.
During our work on cmake build system for GDAL I faced that some
dependency library needs VC2013SP2 on Windows - i.e liblzma, as it needs
C99.
We worked around it by including some logic to cmake, that if VC
compiler is less than specific version the support of dependent features
will be disabled.
https://github.com/nextgis-extra/lib_gdal
https://launchpad.net/~nextgis/+archive/ubuntu/ppa/+packages
Windows builds also using cmake: http://nextgis.ru/en/borsch/
About a month ago I made the hard decision to upgrade my entire MS4W
build environment from VC 2008 to 2015. Driving this is that PHP 7 only
supports 2015.
The only GDAL dependent library not supported for VC 2015 is FileGDB
(I've seen an Esri developer state that 2015 SDK will be released with
their next FileGDB API upgrade).
MrSID 9.5.1 SDK works well with 2015.
I didn't try an ECW 5.x SDK, but the old 3.3 SDK works well with 2015.
Short story is I support future GDAL builds requiring a more recent
Visual Studio compiler (2013+), as other projects are already doing so.
For those packagers cringing in the thought of upgrading their whole
stack: join the club :)
-jeff
Peter Halls
2016-05-05 08:17:24 UTC
Permalink
I am a humble geologist, rather than a Computer Scientist, and do not
pretend to understand all the ins-and-outs of this type of discussion - and
hence read, in the hopes of learning, but otherwise keep quiet!

I suspect that I am also something of a 'throw-back' in that I continue to
use Simula as my primary programming language. Today's Simula compilers
are 'cross-compilers', in that they generate code in c, which is then
compiled and linked with an appropriate c compiler (it actually makes
providing software for multiple platforms a lot easier ...).

Having said that, my only concern with the use of (C++) Vector structures,
rather than c Arrays, would be the mappability required between application
language and implementation language. If the C++ structures are universal,
fine. If not, then either interface code (needing maintenance) or c
alternatives would be needed. My example is based on Simula: it would be
interesting to know how Java or Python might interact.

Best wishes,

Peter
Post by Even Rouault
Post by Mateusz Loskot
Post by Kurt Schwehr
To start off the conversation, I wrote up a doc on changing large C
arrays on the stack to std::vectors to get this data off of the stack
and to simplify initialization.
Since many, if not most, of the ideas rely on availability of the C++11
features,
it might be a good idea to first agree on C++11 as the lowest
required C++ compilation mode.
Let's poll if there are any users who require C++03 at all.
- style and other changes that are not bound to a particular compiler version
- changes potentially dependant of the C++ version
Of the potential issues with requiring C++11, I can think of OSGeo4W. It is
mostly(completely?) built with Visual Studio 2010. And from
https://msdn.microsoft.com/en-us/library/hh567368.aspx , support of C++11 is
only partial in VS 2010
https://lists.osgeo.org/pipermail/discuss/2016-February/015658.html
Potentially we could imagine having GDAL compiled with a later VS version and
have the rest using VS2010, since most (all?) other software in OSGeo4W use it
probably through its C API. Hum, actually that must not be true for OTB that
uses the C++ API I think. Perhaps JÃŒrgen has something to add regarding
this
compiler issue ?
On the other hand regarding dependencies of GDAL, the binary propritary SDKs
with a C++ API could be a problem, although they will likely move on too.
- FileGDB SDK 1.4: available for VS2010, VS2012, VS2013
- ECW SDK 5.2.1: available for VS2010, VS2012, VS2013
- MrSID SDK: I didn't check. Perhaps Kirk can tell us ?
But I'm not sure about the compatibility of C++11 build against non-C++11
builds in the VS realm : can a GDAL C++11 build link against a library built
without C++11 enabled ? Will not there be ABI problems ?
Even
--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
gdal-dev mailing list
http://lists.osgeo.org/mailman/listinfo/gdal-dev
--
----------------------------------------------------------------------------------------------------------------
Peter J Halls, PhD Student, Post-war Reconstruction and Development Unit
(PRDU),
University of York

Snail mail: PRDU, Derwent College, University of York,
Heslington, York YO10 5DD
This message has the status of a private and personal communication
----------------------------------------------------------------------------------------------------------------
Mateusz Loskot
2016-05-05 09:09:48 UTC
Permalink
Post by Even Rouault
Of the potential issues with requiring C++11, I can think of OSGeo4W. It is
mostly(completely?) built with Visual Studio 2010. And from
https://msdn.microsoft.com/en-us/library/hh567368.aspx , support of C++11 is
only partial in VS 2010
VS2013 is the lowest version sensible to consider regarding C++11 support.
Post by Even Rouault
On the other hand regarding dependencies of GDAL, the binary propritary SDKs
with a C++ API could be a problem, although they will likely move on too.
- FileGDB SDK 1.4: available for VS2010, VS2012, VS2013
- ECW SDK 5.2.1: available for VS2010, VS2012, VS2013
AFAICT, GDAL built using VS2015 links against the 5.2.1 version fine.
Post by Even Rouault
- MrSID SDK: I didn't check. Perhaps Kirk can tell us ?
Similarly to the ECW above, version 9.1.0 links fine too.
Post by Even Rouault
But I'm not sure about the compatibility of C++11 build against non-C++11
builds in the VS realm : can a GDAL C++11 build link against a library built
without C++11 enabled ? Will not there be ABI problems ?
Although mixing C run-time libraries is a bad idea generally,
kosher C APIs should be fine.

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
Even Rouault
2016-05-05 09:28:14 UTC
Permalink
Post by Mateusz Loskot
Post by Even Rouault
Of the potential issues with requiring C++11, I can think of OSGeo4W. It
is mostly(completely?) built with Visual Studio 2010. And from
https://msdn.microsoft.com/en-us/library/hh567368.aspx , support of C++11
is only partial in VS 2010
VS2013 is the lowest version sensible to consider regarding C++11 support.
Post by Even Rouault
On the other hand regarding dependencies of GDAL, the binary propritary
SDKs with a C++ API could be a problem, although they will likely move
on too. - FileGDB SDK 1.4: available for VS2010, VS2012, VS2013
- ECW SDK 5.2.1: available for VS2010, VS2012, VS2013
AFAICT, GDAL built using VS2015 links against the 5.2.1 version fine.
Do you link against the VS2013 .lib ?
Post by Mateusz Loskot
Post by Even Rouault
- MrSID SDK: I didn't check. Perhaps Kirk can tell us ?
Similarly to the ECW above, version 9.1.0 links fine too.
Post by Even Rouault
But I'm not sure about the compatibility of C++11 build against non-C++11
builds in the VS realm : can a GDAL C++11 build link against a library
built without C++11 enabled ? Will not there be ABI problems ?
Although mixing C run-time libraries is a bad idea generally,
kosher C APIs should be fine.
I'm not too worried about C ABI. My mention of C++03 ABI vs C++11 ABI was for
the dependencies with those SDK that we use through their C++ API. In GCC
world, with same compiler but with different -std=... flags, there have been
troubles ( https://gcc.gnu.org/wiki/Cxx11AbiCompatibility ,
https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_dual_abi.html ), so I
was wondering how the situation is in the VS world.
Actually since I mention this, the issue does apply to Linux too. Looking at
the C++11 Linux build (
https://github.com/rouault/gdal_coverage/blob/trunk_gcc4.8_stdc11/.travis.yml
) , I see I couldn't build against FileGDB with -std=c++11. This was a
compilation issue with its headers, rather than a link/runtime issue.
--
Spatialys - Geospatial professional services
http://www.spatialys.com
Mateusz Loskot
2016-05-05 10:15:14 UTC
Permalink
Post by Even Rouault
Post by Mateusz Loskot
Post by Even Rouault
Of the potential issues with requiring C++11, I can think of OSGeo4W. It
is mostly(completely?) built with Visual Studio 2010. And from
https://msdn.microsoft.com/en-us/library/hh567368.aspx , support of C++11
is only partial in VS 2010
VS2013 is the lowest version sensible to consider regarding C++11 support.
Post by Even Rouault
On the other hand regarding dependencies of GDAL, the binary propritary
SDKs with a C++ API could be a problem, although they will likely move
on too. - FileGDB SDK 1.4: available for VS2010, VS2012, VS2013
- ECW SDK 5.2.1: available for VS2010, VS2012, VS2013
AFAICT, GDAL built using VS2015 links against the 5.2.1 version fine.
Do you link against the VS2013 .lib ?
Yes, namely these:

ls -1 ERDAS_ECW_JPEG_2000\lib\vc120\Win32
NCSEcw.lib
NCSEcwd.lib
NCSEcwS.lib
NCSEcwSd.lib
Post by Even Rouault
Post by Mateusz Loskot
Post by Even Rouault
- MrSID SDK: I didn't check. Perhaps Kirk can tell us ?
Similarly to the ECW above, version 9.1.0 links fine too.
Post by Even Rouault
But I'm not sure about the compatibility of C++11 build against non-C++11
builds in the VS realm : can a GDAL C++11 build link against a library
built without C++11 enabled ? Will not there be ABI problems ?
Although mixing C run-time libraries is a bad idea generally,
kosher C APIs should be fine.
I'm not too worried about C ABI. My mention of C++03 ABI vs C++11 ABI was for
the dependencies with those SDK that we use through their C++ API
Visual C++ does not promise ABI compatibility for anything but a
"portable type" [1]
- a C built-in type or a struct that contains only C built-in types

Mixing binaries built with different compiler versions or even
compiler/linker settings
is not recommended [2]. It might or might not work :)

[1] https://msdn.microsoft.com/en-us/library/hh438475.aspx
[2] https://msdn.microsoft.com/en-us/library/bb531344.aspx

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
Damian Dixon
2016-05-05 15:48:18 UTC
Permalink
In the VS World...

If you delete C/C++ memory/objects in a DLL that has been compiled with the
same VS compiler you are ok.

If you attempt to delete a memory/object in DLL/application compiled with a
different version of the VS compiler all bets are off.

For example:

Allocate an object in VS2010 compiled code delete in VS2015 compiled code,
you will likely crash.


You will have to distribute the runtime distribution DLLs for the versions
of VS that you used or your 3rd party DLLs used.

Regards
Damian
Post by Mateusz Loskot
Post by Mateusz Loskot
Post by Even Rouault
Of the potential issues with requiring C++11, I can think of OSGeo4W.
It
Post by Mateusz Loskot
Post by Even Rouault
is mostly(completely?) built with Visual Studio 2010. And from
https://msdn.microsoft.com/en-us/library/hh567368.aspx , support of
C++11
Post by Mateusz Loskot
Post by Even Rouault
is only partial in VS 2010
VS2013 is the lowest version sensible to consider regarding C++11
support.
Post by Mateusz Loskot
Post by Even Rouault
On the other hand regarding dependencies of GDAL, the binary propritary
SDKs with a C++ API could be a problem, although they will likely move
on too. - FileGDB SDK 1.4: available for VS2010, VS2012, VS2013
- ECW SDK 5.2.1: available for VS2010, VS2012, VS2013
AFAICT, GDAL built using VS2015 links against the 5.2.1 version fine.
Do you link against the VS2013 .lib ?
Post by Mateusz Loskot
Post by Even Rouault
- MrSID SDK: I didn't check. Perhaps Kirk can tell us ?
Similarly to the ECW above, version 9.1.0 links fine too.
Post by Even Rouault
But I'm not sure about the compatibility of C++11 build against
non-C++11
Post by Mateusz Loskot
Post by Even Rouault
builds in the VS realm : can a GDAL C++11 build link against a library
built without C++11 enabled ? Will not there be ABI problems ?
Although mixing C run-time libraries is a bad idea generally,
kosher C APIs should be fine.
I'm not too worried about C ABI. My mention of C++03 ABI vs C++11 ABI was for
the dependencies with those SDK that we use through their C++ API. In GCC
world, with same compiler but with different -std=... flags, there have been
troubles ( https://gcc.gnu.org/wiki/Cxx11AbiCompatibility ,
https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_dual_abi.html ), so I
was wondering how the situation is in the VS world.
Actually since I mention this, the issue does apply to Linux too. Looking at
the C++11 Linux build (
https://github.com/rouault/gdal_coverage/blob/trunk_gcc4.8_stdc11/.travis.yml
) , I see I couldn't build against FileGDB with -std=c++11. This was a
compilation issue with its headers, rather than a link/runtime issue.
--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
gdal-dev mailing list
http://lists.osgeo.org/mailman/listinfo/gdal-dev
Even Rouault
2016-05-06 14:00:07 UTC
Permalink
Post by Even Rouault
- ECW SDK 5.2.1: available for VS2010, VS2012, VS2013
I got information that the next SDK 5.3 should support VS2012, 2013 and 2015
(so no more 2010) + GCC 5
--
Spatialys - Geospatial professional services
http://www.spatialys.com
Joaquim Luis
2016-05-06 14:20:37 UTC
Permalink
On Fri, 06 May 2016 15:00:07 +0100, Even Rouault
Post by Even Rouault
Post by Even Rouault
- ECW SDK 5.2.1: available for VS2010, VS2012, VS2013
I got information that the next SDK 5.3 should support VS2012, 2013 and 2015
(so no more 2010) + GCC 5
And one can also build SDK 3.3 with all of those compilers (I did).
Mateusz Loskot
2016-05-06 14:38:04 UTC
Permalink
Post by Joaquim Luis
On Fri, 06 May 2016 15:00:07 +0100, Even Rouault
Post by Even Rouault
Post by Even Rouault
- ECW SDK 5.2.1: available for VS2010, VS2012, VS2013
I got information that the next SDK 5.3 should support VS2012, 2013 and 2015
(so no more 2010) + GCC 5
And one can also build SDK 3.3 with all of those compilers (I did).
I do it too, works well indeed.

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
Jeff McKenna
2016-05-06 14:55:31 UTC
Permalink
Post by Mateusz Loskot
Post by Joaquim Luis
On Fri, 06 May 2016 15:00:07 +0100, Even Rouault
Post by Even Rouault
Post by Even Rouault
- ECW SDK 5.2.1: available for VS2010, VS2012, VS2013
I got information that the next SDK 5.3 should support VS2012, 2013 and 2015
(so no more 2010) + GCC 5
And one can also build SDK 3.3 with all of those compilers (I did).
I do it too, works well indeed.
same.

-jeff
--
Jeff McKenna
MapServer Consulting and Training Services
http://www.gatewaygeomatics.com/
Damian Dixon
2016-05-05 15:41:13 UTC
Permalink
I'm currently stuck on the following compilers:

- Visual Studio 2010 SP1
- GCC 4.7

C++11 is only partially supported with these compilers.

Regards
Damian
Post by Mateusz Loskot
Post by Kurt Schwehr
To start off the conversation, I wrote up a doc on changing large C
arrays on the stack to std::vectors to get this data off of the stack and
to simplify initialization.
Since many, if not most, of the ideas rely on availability of the C++11
features,
it might be a good idea to first agree on C++11 as the lowest
required C++ compilation mode.
Let's poll if there are any users who require C++03 at all.
Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
_______________________________________________
gdal-dev mailing list
http://lists.osgeo.org/mailman/listinfo/gdal-dev
David Strip
2016-05-05 14:14:34 UTC
Permalink
_______________________________________________
gdal-dev mailing list
gdal-***@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/gdal-dev
Even Rouault
2016-05-05 14:31:05 UTC
Permalink
<description of approach using std::vector>
It is possible to change the size of the vector later on in the code
Vector has some storage overhead and bookkeeping that has to be done (but
References that explain this? Resizing the array could break anything that
was using a C style array pointer to the vector’s data
Drawbacks one and three can be eliminated by deriving a class from vector
that hides resize, so there really is only the single drawback of storage
overhead and bookkeeping, which are often minor.
I'd not find it very attractive to create our own class just to prevent
resizing for such a simple thing as an array of integers... "std::vector<int>
oVals(256, 0)" is very readable. If it must not be resized for some reason, a
comment should do.

One thing to keep in mind however is the potential impact of heap allocation
in performance sensitive code paths.
--
Spatialys - Geospatial professional services
http://www.spatialys.com
Mateusz Loskot
2016-05-05 15:14:01 UTC
Permalink
Post by Even Rouault
Post by Kurt Schwehr
It is possible to change the size of the vector later on in the code
Vector has some storage overhead and bookkeeping that has to be done (but
References that explain this? Resizing the array could break anything that
was using a C style array pointer to the vector’s data
Drawbacks one and three can be eliminated by deriving a class from vector
that hides resize, so there really is only the single drawback of storage
overhead and bookkeeping, which are often minor.
I'd not find it very attractive to create our own class just to prevent
resizing for such a simple thing as an array of integers... "std::vector<int>
oVals(256, 0)" is very readable. If it must not be resized for some reason, a
comment should do.
An assertion, you mean :)
Post by Even Rouault
One thing to keep in mind however is the potential impact of heap allocation
in performance sensitive code paths.
Indeed.
I would divide Kurt's proposal to several phases, roughly:
1. Switch to C++11
2. Refactor: clean-up, clarify, simplify, make it less error prone
using C++11 idioms and features (i.e. int[256] => std::array<int,
256>)
3. Update coding guidelines
4. Measure and optimise using C++11 features with potential
functional/behavioural changes (yes, switch from stack to heap
allocation drags a behavioural change, e.g. exceptions).

Although it looks like a long and boring process, points 2 and 4 are
optional, IMO.
However, mixing plain refactoring with functional and behaviour
changes is a bad idea.

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
David Strip
2016-05-05 16:04:15 UTC
Permalink
_______________________________________________
gdal-dev mailing list
gdal-***@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/gdal-dev
Ari Jolma
2016-05-09 08:15:22 UTC
Permalink
Post by Kurt Schwehr
Hi all,
If you've been watching the timeline on trac, you have probably seen a
large number of cleanup CLs from me. It's definitely past time to get
some discussion going on these changes. If the community likes these,
we can add them to rfc8_devguide
<https://trac.osgeo.org/gdal/wiki/rfc8_devguide>.
It is very minimal right now. Good style guides tend to be quite large,
e.g., https://google.github.io/styleguide/cppguide.html or
http://geosoft.no/development/cppstyle.html

Is there something GDAL could reuse?

Ari
Mateusz Loskot
2016-05-09 13:05:10 UTC
Permalink
Post by Kurt Schwehr
Hi all,
If you've been watching the timeline on trac, you have probably seen a large
number of cleanup CLs from me. It's definitely past time to get some
discussion going on these changes. If the community likes these, we can add
them to rfc8_devguide.
It is very minimal right now. Good style guides tend to be quite large,
e.g., https://google.github.io/styleguide/cppguide.html or
http://geosoft.no/development/cppstyle.html
Good C++ guidelines that fit on a single page is all you need
these days, after Bjarne Stroustrup
https://parasol.tamu.edu/people/bs/622-GP/C++11-style.pdf

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
Joaquim Luis
2016-05-09 15:14:39 UTC
Permalink
Hi,

There is one aspect of the coding style that I honestly do not understand.
Why continuing to recommend the 80 chars line width? I don't by the
readability argument, well on the contrary, if because of it the result is
an excess 'verticalization' of the code it becomes much harder to read.

I don't know for you guys but when I'm forced to permanently scroll up and
down it becomes really much harder to study a code. For me the ideal is
that a function holds in a single screen size, but ofc without piling up
everything to reach that goal.

A little example from gdaldem_lib.cpp (line 240)

****************** Current code *********************************

for ( i = 0; i < 2 && i < nYSize; i++)
{
if( GDALRasterIO( hSrcBand,
GF_Read,
0, i,
nXSize, 1,
pafThreeLineWin + i * nXSize,
nXSize, 1,
GDT_Float32,
0, 0) != CE_None )
{
eErr = CE_Failure;
goto end;
}
}
******************************************************************************


************ What I think would be reasonable
********************************

for (i = 0; i < 2 && i < nYSize; i++) {
if (GDALRasterIO(hSrcBand, GF_Read, 0, i, nXSize, 1,
pafThreeLineWin + i * nXSize, nXSize, 1,
GDT_Float32, 0, 0) != CE_None ) {
eErr = CE_Failure;
goto end;
}
}
******************************************************************************

7 lines instead of 15, and is it less readable (in a text editor, not in a
mail client)?

Joaquim
Post by Mateusz Loskot
Post by Kurt Schwehr
Hi all,
If you've been watching the timeline on trac, you have probably seen a large
number of cleanup CLs from me. It's definitely past time to get some
discussion going on these changes. If the community likes these, we can add
them to rfc8_devguide.
It is very minimal right now. Good style guides tend to be quite large,
e.g., https://google.github.io/styleguide/cppguide.html or
http://geosoft.no/development/cppstyle.html
Good C++ guidelines that fit on a single page is all you need
these days, after Bjarne Stroustrup
https://parasol.tamu.edu/people/bs/622-GP/C++11-style.pdf
Best regards,
Ari Jolma
2016-05-09 15:44:48 UTC
Permalink
I like one statement per line because it helps walking through the code
with gdb; you can see the whole statement that is going to be executed.

I also share the idea that seeing a whole subroutine or a logical part
at once helps understanding the code.

Ari
Post by Joaquim Luis
Hi,
There is one aspect of the coding style that I honestly do not
understand. Why continuing to recommend the 80 chars line width? I
don't by the readability argument, well on the contrary, if because of
it the result is an excess 'verticalization' of the code it becomes
much harder to read.
I don't know for you guys but when I'm forced to permanently scroll up
and down it becomes really much harder to study a code. For me the
ideal is that a function holds in a single screen size, but ofc
without piling up everything to reach that goal.
A little example from gdaldem_lib.cpp (line 240)
****************** Current code *********************************
for ( i = 0; i < 2 && i < nYSize; i++)
{
if( GDALRasterIO( hSrcBand,
GF_Read,
0, i,
nXSize, 1,
pafThreeLineWin + i * nXSize,
nXSize, 1,
GDT_Float32,
0, 0) != CE_None )
{
eErr = CE_Failure;
goto end;
}
}
******************************************************************************
************ What I think would be reasonable
********************************
for (i = 0; i < 2 && i < nYSize; i++) {
if (GDALRasterIO(hSrcBand, GF_Read, 0, i, nXSize, 1,
pafThreeLineWin + i * nXSize, nXSize, 1,
GDT_Float32, 0, 0) != CE_None ) {
eErr = CE_Failure;
goto end;
}
}
******************************************************************************
7 lines instead of 15, and is it less readable (in a text editor, not
in a mail client)?
Joaquim
Post by Mateusz Loskot
Post by Kurt Schwehr
Hi all,
If you've been watching the timeline on trac, you have probably seen a large
number of cleanup CLs from me. It's definitely past time to get some
discussion going on these changes. If the community likes these, we can add
them to rfc8_devguide.
It is very minimal right now. Good style guides tend to be quite large,
e.g., https://google.github.io/styleguide/cppguide.html or
http://geosoft.no/development/cppstyle.html
Good C++ guidelines that fit on a single page is all you need
these days, after Bjarne Stroustrup
https://parasol.tamu.edu/people/bs/622-GP/C++11-style.pdf
Best regards,
_______________________________________________
gdal-dev mailing list
http://lists.osgeo.org/mailman/listinfo/gdal-dev
Joaquim Luis
2016-05-09 16:43:08 UTC
Permalink
Post by Ari Jolma
I like one statement per line because it helps walking through the code
with gdb; you can see the whole statement that is going to be executed.
Fully agree, and my example respects that.
Post by Ari Jolma
I also share the idea that seeing a whole subroutine or a logical part
at once helps understanding the code.
Ari
Post by Joaquim Luis
Hi,
There is one aspect of the coding style that I honestly do not
understand. Why continuing to recommend the 80 chars line width? I
don't by the readability argument, well on the contrary, if because of
it the result is an excess 'verticalization' of the code it becomes
much harder to read.
I don't know for you guys but when I'm forced to permanently scroll up
and down it becomes really much harder to study a code. For me the
ideal is that a function holds in a single screen size, but ofc without
piling up everything to reach that goal.
A little example from gdaldem_lib.cpp (line 240)
****************** Current code *********************************
for ( i = 0; i < 2 && i < nYSize; i++)
{
if( GDALRasterIO( hSrcBand,
GF_Read,
0, i,
nXSize, 1,
pafThreeLineWin + i * nXSize,
nXSize, 1,
GDT_Float32,
0, 0) != CE_None )
{
eErr = CE_Failure;
goto end;
}
}
******************************************************************************
************ What I think would be reasonable
********************************
for (i = 0; i < 2 && i < nYSize; i++) {
if (GDALRasterIO(hSrcBand, GF_Read, 0, i, nXSize, 1,
pafThreeLineWin + i * nXSize, nXSize, 1,
GDT_Float32, 0, 0) != CE_None ) {
eErr = CE_Failure;
goto end;
}
}
******************************************************************************
7 lines instead of 15, and is it less readable (in a text editor, not
in a mail client)?
Joaquim
Post by Mateusz Loskot
Post by Kurt Schwehr
Hi all,
If you've been watching the timeline on trac, you have probably seen a large
number of cleanup CLs from me. It's definitely past time to get some
discussion going on these changes. If the community likes these, we can add
them to rfc8_devguide.
It is very minimal right now. Good style guides tend to be quite large,
e.g., https://google.github.io/styleguide/cppguide.html or
http://geosoft.no/development/cppstyle.html
Good C++ guidelines that fit on a single page is all you need
these days, after Bjarne Stroustrup
https://parasol.tamu.edu/people/bs/622-GP/C++11-style.pdf
Best regards,
_______________________________________________
gdal-dev mailing list
http://lists.osgeo.org/mailman/listinfo/gdal-dev
_______________________________________________
gdal-dev mailing list
http://lists.osgeo.org/mailman/listinfo/gdal-dev
Kurt Schwehr
2016-05-09 17:09:33 UTC
Permalink
Please fork the 80 col style guide to a new thread. Yes, there are lots of
issues with the coding style, but can we do one thing at a time? If you
are really motivated, why not write a proposal like mine and talk through
the issues of different column constraints (there are good reasons for the
many different ways and related issues). Then start a new thread with it.
Post by Ari Jolma
I like one statement per line because it helps walking through the code
Post by Ari Jolma
with gdb; you can see the whole statement that is going to be executed.
Fully agree, and my example respects that.
I also share the idea that seeing a whole subroutine or a logical part at
Post by Ari Jolma
once helps understanding the code.
Ari
Post by Joaquim Luis
Hi,
There is one aspect of the coding style that I honestly do not
understand. Why continuing to recommend the 80 chars line width? I don't by
the readability argument, well on the contrary, if because of it the result
is an excess 'verticalization' of the code it becomes much harder to read.
I don't know for you guys but when I'm forced to permanently scroll up
and down it becomes really much harder to study a code. For me the ideal is
that a function holds in a single screen size, but ofc without piling up
everything to reach that goal.
A little example from gdaldem_lib.cpp (line 240)
****************** Current code *********************************
for ( i = 0; i < 2 && i < nYSize; i++)
{
if( GDALRasterIO( hSrcBand,
GF_Read,
0, i,
nXSize, 1,
pafThreeLineWin + i * nXSize,
nXSize, 1,
GDT_Float32,
0, 0) != CE_None )
{
eErr = CE_Failure;
goto end;
}
}
******************************************************************************
************ What I think would be reasonable
********************************
for (i = 0; i < 2 && i < nYSize; i++) {
if (GDALRasterIO(hSrcBand, GF_Read, 0, i, nXSize, 1,
pafThreeLineWin + i * nXSize, nXSize, 1,
GDT_Float32, 0, 0) != CE_None ) {
eErr = CE_Failure;
goto end;
}
}
******************************************************************************
7 lines instead of 15, and is it less readable (in a text editor, not in a
mail client)?
Joaquim
Post by Mateusz Loskot
Post by Kurt Schwehr
Hi all,
If you've been watching the timeline on trac, you have probably seen a large
number of cleanup CLs from me. It's definitely past time to get some
discussion going on these changes. If the community likes these, we can add
them to rfc8_devguide.
It is very minimal right now. Good style guides tend to be quite large,
e.g., https://google.github.io/styleguide/cppguide.html or
http://geosoft.no/development/cppstyle.html
Good C++ guidelines that fit on a single page is all you need
these days, after Bjarne Stroustrup
https://parasol.tamu.edu/people/bs/622-GP/C++11-style.pdf
Best regards,
_______________________________________________
gdal-dev mailing list
http://lists.osgeo.org/mailman/listinfo/gdal-dev
_______________________________________________
gdal-dev mailing list
http://lists.osgeo.org/mailman/listinfo/gdal-dev
_______________________________________________
gdal-dev mailing list
http://lists.osgeo.org/mailman/listinfo/gdal-dev
--
--
http://schwehr.org
Mateusz Loskot
2016-05-09 16:02:06 UTC
Permalink
Post by Joaquim Luis
Hi,
There is one aspect of the coding style that I honestly do not understand.
Why continuing to recommend the 80 chars line width?
Perhaps, paraphrasing Kevlin Henney, because
not everyone falls in love with high-resolution screens
and not everyone can actually now project the whole of the Serengeti :)

[1] https://vimeo.com/97329157

Best regards,
--
Mateusz Loskot, http://mateusz.loskot.net
Joaquim Luis
2016-05-09 16:42:13 UTC
Permalink
I do all my programing in laptops, and yes I have a 15 inches HiRes screen
but I also have my eyes, well, weakened (and 54 years old) so I make the
fonts larger. Having to permanently scroll up and down is far worst than
any perception purity.
Post by Mateusz Loskot
Post by Joaquim Luis
There is one aspect of the coding style that I honestly do not understand.
Why continuing to recommend the 80 chars line width?
Perhaps, paraphrasing Kevlin Henney, because
not everyone falls in love with high-resolution screens
and not everyone can actually now project the whole of the Serengeti :)
[1] https://vimeo.com/97329157
Best regards,
Continue reading on narkive:
Loading...