Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove some unused local variables and functions #752

Merged
merged 4 commits into from
Mar 10, 2017

Conversation

stweil
Copy link
Contributor

@stweil stweil commented Mar 9, 2017

This fixes several compiler warnings.

This fixes a compiler warning:

lstm/input.cpp:141:7: warning: unused variable 'width' [-Wunused-variable]

Signed-off-by: Stefan Weil <sw@weilnetz.de>
This fixes a compiler warning:

api/baseapi.cpp:1621:17: warning:
 variable 'font_name' set but not used [-Wunused-but-set-variable]

Signed-off-by: Stefan Weil <sw@weilnetz.de>
This fixes a compiler warning:

ccutil/scanutils.cpp:284:7: warning:
 variable 'sign' set but not used [-Wunused-but-set-variable]

Signed-off-by: Stefan Weil <sw@weilnetz.de>
This fixes a compiler warning:

classify/trainingsampleset.cpp:510:13: warning:
 'Pix* tesseract::DebugSample(const UNICHARSET&, tesseract::TrainingSample*)'
 defined but not used [-Wunused-function]

Signed-off-by: Stefan Weil <sw@weilnetz.de>
@@ -507,22 +507,6 @@ bool TrainingSampleSet::DeleteableSample(const TrainingSample* sample) {
return sample == NULL || sample->class_id() < 0;
}

static Pix* DebugSample(const UNICHARSET& unicharset,
Copy link
Collaborator

@amitdo amitdo Mar 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole 'classify' directory will most likely be removed by Ray (it is part of the old engine). There is no point in applying any change to files under this directory.

@stweil
Copy link
Contributor Author

stweil commented Mar 9, 2017

The whole 'classify' directory will most likely be removed by Ray (it is part of the old engine). There is no point in applying any change to files under this directory.

I disagree. Maybe that directory will be removed in the future. That's not a good reason why unused code should not be removed now. Maybe we could support a Tesseract 4 with both old and new recognizer engine at least for the next year (thus eliminating the need to support Tesseract 3 for a longer time span). I'd appreciate that scenario, but if we prevent any code maintenance of the old engine in Tesseract 4, we'll never know whether that might be possible.

See also #744 which I'd apply for the same reason. My code for big endian support (#703) also changes both recognizers.

@amitdo
Copy link
Collaborator

amitdo commented Mar 9, 2017

If you really want to piss @theraysmith off, then fine...

@stweil
Copy link
Contributor Author

stweil commented Mar 9, 2017

I don't. As far as I have understood Ray, he wants a Tesseract which works really good. LSTM in Tesseract 4 is a large step forward. Nevertheless, there is obviously still some need for the old engine until we can be sure that LSTM outperforms it in all OCR applications (see issue #707). Based on my experience I'd try to keep both engines for some time to allow easy verification of the LSTM performance. In issue #518, Ray said that he wants to remove the old engine, but is open for a discussion based on facts, and he also said what would have to be done if the old engine were kept.

@amitdo
Copy link
Collaborator

amitdo commented Mar 9, 2017

I understand Ray's messages on this topic differently.

Zdenko will decide...

Peace :-)

@amitdo
Copy link
Collaborator

amitdo commented Mar 9, 2017

@egorpugin
Is there no good alternative to AppVeyor?

@egorpugin
Copy link
Contributor

I don't know.
The issue is currently in cppan. I've just found it, probably will fix it tomorrow or till Monday.

@zdenop zdenop merged commit fe986df into tesseract-ocr:master Mar 10, 2017
@zdenop
Copy link
Contributor

zdenop commented Mar 10, 2017

IMO removing unused function from directory, that should be removed, is not big issue.

@stweil stweil deleted the warn branch March 10, 2017 12:13
@theraysmith
Copy link
Contributor

OK, I only skimmed this to get the gist.
Yes I'm open to discussion on the future of the legacy engine in 4.00 and beyond, but my aim is to prove to the community that it can be safely removed, so my favorite kind of discussion is "it doesn't work well for this image" with example.
I'm not against removal of unused code from the dead code.
What I'm not keen on is major edits to the dead code that don't remove dead code.

I have a commit coming (next week?) that fulfills a long-standing request - to be able to load from a trainneddata file in memory (or via a file-reader function pointer). This involves a major edit to the dead code, but I had to do it to keep Android portability.
This will also showcase my clean and simple fix to the endian issue that Stefan raised.
I haven't yet done the endian fix to the LSTM code, but it is easy now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants