C++ standard version? about rawspeed

[klauspost]

I don't have an official stance, but I would prefer a conservative approach to C++ features. We need to support GCC, MSVC 2013+, Clang.

Generally I prefer to stay close to C, but only use the "++" that gives clarity.

Which leaks are you seeing?

[LebedevRI]

GCC

'Fully' supports C++11 since GCC 4.7 (GCC 4.7.0 - March 22, 2012)

Clang

'Fully' supports C++11 since Clang 3.3 (LLVM 3.3 - June 19, 2013)

MSVC 2013+

According to
https://blogs.msdn.microsoft.com/vcblog/2013/12/02/c1114-core-language-features-in-vs-2013-and-the-nov-2013-ctp/ and https://msdn.microsoft.com/en-us/library/hh567368.aspx the support is not bad.

Which leaks are you seeing?

I'm sorry, i can not find those exact leaks right now, IIRC it was the linearization tables in some decoders that were leaking.

However the actual amount of leaks is probably much higher, because of things like darktable-org/darktable@6eadbde
I needed to do such changes to port darktable to musl libc.
The problem here is that e.g. in this case i didn't think what will happen if it throws, and there is no delete in catch section, so it will leak.
And so on.

From what i understand, all this can be completely avoided by using proper RAII via std::unique_ptr.

[LebedevRI]

Addendum: found one, but there are/were more like this:

Direct leak of 7714 byte(s) in 2 object(s) allocated from:
    0 0x7f1186764d70 in operator new[](unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.3+0xc2d70)
    1 0x7f11864d9cd1 in RawSpeed::DngDecoder::decodeRawInternal() /home/lebedevri/darktable/src/external/rawspeed/RawSpeed/DngDecoder.cpp:398
    2 0x7f1186484177 in RawSpeed::RawDecoder::decodeRaw() /home/lebedevri/darktable/src/external/rawspeed/RawSpeed/RawDecoder.cpp:682
    3 0x7f1186261b57 in dt_imageio_open_rawspeed /home/lebedevri/darktable/src/common/imageio_rawspeed.cc:148
...

[axxel]

As a non-contributor (as of yet) but a user closely following the development of rawspeed, I'd also very much appreciate the use of (at least) c++11. While debugging a segfault I had a closer look at some of the internals and also noticed quite a few places where state of the art c++ would help improving clarity. So in this regard, clarity is exactly what could be gained. And replacing pretty much all explicit new/delete statements is one example of what people like Herb Sutter preach to improve any old c++ code base.

Adding to LebedebRI's leak example: I had a look at a couple files and found that e.g. in CR2Decoder.cpp 3 of 3 and in DngDecoder.cpp 1 of 2 new occurrences leak.

Regarding the mentioned necessity to reduce stack allocations to support musl libc, I would suggest to use another approach than using new+delete or even unique_ptr on the 'client' side of the class interface, but rather reduce the stack usage inside the class. If I see it correctly, one of the 'problematic' code snippets is just one line: 'HuffmanTable huff[4];' in the LJpegDecrompressor. I'd prefer to replace that with a std::vector and leave the instantiations of the different decoders on the stack as they were before. So, don't apply the patches similar to f73cc07. I think that is even better than the use of std::unique_ptr, because it is more local (possibly guarded by an #ifdef LIB_MUSL kind if macro) and even safer at taking care of memory management. And you don't have to remember which class is ok to allocate on the stack and which is not.

[axxel]

@klauspost : would you please give your opinion on the matter of accepting changes requiring c++11 or not? Thanks.

[LebedevRI]

I feel like saying that if it is a choice between "using c and minimal amount of c++ only where really needed" and "just rewrite it all with c++22 because it is so cool", i will settle with no c++11 :)

[axxel]

Meaning, you changed your mind regarding your suggested introduction of std::unique_ptr? Did I give the impression that I would want to completely "rewrite" it, just to be cool? ;). And what means "where c++ is really needed"? librawspeed could most certainly be done in plain old c :).

[axxel]

I just noticed (by chance) that the current code base is already using c++11 features: d2230a4. So I would argue, the decision has been made a while back already and the answer was: c++11 is fine. Agreed?

[LebedevRI]

@klauspost hey. just want to let you know that if no further communication happens, we will proceed with the simple clone. Explicit answer would be better though.