RAII

[rfschtkt]

Use the RAII idiom, e.g., to help prevent memory leaks.

[stweil]

Build error: https://travis-ci.org/tesseract-ocr/tesseract/jobs/230329088#L290.

[rfschtkt]

Yes, I think it's a bug in that g++ version (4.8.4). I had successfully built with 5.4.0. I'll try again with an explicit move constructor...

[Shreeshrii]

Please use syntax which will work with all of the following. Supported Compilers - GCC 4.8 and above - Clang 3.4 and above - MSVC 2015, 2017 ShreeDevi

…

____________________________________________________________ भजन - कीर्तन - आरती @ http://bhajans.ramparivar.com

On Tue, May 9, 2017 at 6:18 PM, rfschtkt ***@***.***> wrote: Yes, I think it's a bug in that g++ version. I had successfully built with 5.4.0. I'll try again with an explicit move constructor... — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#897 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE2_o-UsQwL8k-hPWd3NDI4loF080sLUks5r4GCXgaJpZM4NVPlU> .

[rfschtkt]

Are they all tested? I think that it was valid C++11, though.

Amended, pushed...

And again...

[amitdo]

Since you change the API, I think we need Ray's approval for this PR.

[rfschtkt]

If that's public API and cast in stone, maybe there should be two versions, with the backward-compatible version calling the safe version with release(). Or the leak could be fixed ad hoc, but that's not really progress.

(Added) Is the public API not tested?

(Added) I don't see any documentation mentioning ccmain/resultiterator.h as part of the API? Ah, apparently the result of tesseract::TessBaseAPI::GetIterator (), that sounds very API-ish.

[amitdo]

I would start with fixing the leak with the current API.
If you want to do it, open a new PR.

Fixing a leak is still progress...

[rfschtkt]

Maybe tomorrow, but I would still suggest to add a new version and deprecate the current one, because this is C++, and RAII is one of C++'s most useful idioms (to put it mildly). That's why I made a new thread called RAII, not lookifoundaleak...

[egorpugin]

Oh, btw.
Isn't GetUTF8Text() should return C++-ish std::string? Instead of that stupid non-owning char* memory ptr.
It would be way better fix for all of this.

[rfschtkt]

Yes, but it can also return nullptr, and I hadn't yet figured out whether there's a difference with the empty string. Adding to a STRING is apparently the same for nullptr and empty string, but maybe not other applications. And there's at least one use of release() that would then instead have to make a new copy.

(Correction) I meant strdup() instead of strcpy(), but that's even more expensive because it needs two passes, so let's stick with "make a new copy".

(Added) virtual ResultIterator::GetUTF8Text() hides non-virtual LTRResultIterator::GetUTF8Text, that doesn't look great...

[rfschtkt]

Amended without API change, checks successful.

[amitdo]

LGTM. @stweil?

[stweil]

I plan to run a test with Valgrind later (~ 1 day) and will add my review comment then.

[amitdo]

but I would still suggest to add a new version and deprecate the current one, because this is C++, and RAII is one of C++'s most useful idioms (to put it mildly).

The new version of this PR uses RAII technique, without an API change :)
http://stackoverflow.com/questions/29372976/is-stdunique-ptr-an-application-of-raii

[rfschtkt]

There seems to be another leak in void POLY_BLOCK::fill(ScrollView* window, ScrollView::Color colour), which doesn't do delete segments;. But I've had enough fun for now...

[amitdo]

I've had enough fun for now...

😆

[rfschtkt]

Well, it seemed to be leaked, from comparing with other uses, although I'm not 100% certain...

[stweil]

Valgrind test passed (no change of alloc / free for OCR of simple image).

[rfschtkt]

Rebased.