-
Notifications
You must be signed in to change notification settings - Fork 9.5k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
The function pointers and callbacks file_reader_, file_writer_, checkpointer_reader_ and checkpoint_writer_ are always set to the same values. Replacing them by direct function calls simplifies the code and allows removing more code from tesscallback.h. Signed-off-by: Stefan Weil <[email protected]>
- Loading branch information
Showing
5 changed files
with
19 additions
and
172 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
563a171
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit caused API breaking...
563a171
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LSTMTrainer is not part of the 4.0 API (I searched the include files in /usr/include/tesseract), so I am not sure whether this is really relevant. Nevertheless I reverted that commit now for 4.1.
563a171
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stweil: problem is related to
tesscallback.h
- this file is part of API and also report related to API breaking is for this file... So only changes of this file (after 4.1.0-rc4) should be reverted.563a171
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compatibility report does not complain because tesscallback.h was changed but because symbols in libtesseract.so.4.0.1 were removed. As tesscallback.h declares template classes, those symbols depend on tesscallback.h and the class used for the template (here LSTMTrainer). I wonder whether these special reported high priority problems with removed symbols are relevant at all.
tesscallback.h is only part of the API header files because it is required by other header files. Third party applications won't use it.
563a171
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I mean removal of symbols with modification of tesscallback.h.
My understanding is that we agreed that we will make 4.1 release backward compatible and as reference we use abi-laboratory report.
The current result is this:
so more commit should be reverted...
563a171
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure whether more commits should be reverted.
How often is the compatibility report updated? Ideally it should be possible to reproduce it locally at any time, but my results look different and did not complain about removed symbols (maybe because the removed symbols were not exposed via public API header files before).
Debian will start with a new stable release in a few days, and as far as I see that new release will include Tesseract 4.0 for the next few years. Let's discuss at issue #1423 what consequences that has for Tesseract 4.1.
563a171
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am able to run report locally on my linux machine (no bid deal, just machine is too slow). abi-laboratory.pro run it 2019-07-02 06:39.
Removal of any function from tesscallback.h will be reported as problem as it is part of API... Removing unused functionalities at this stage of release has no benefit for end users...
563a171
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Earlier removals like 3ae4069 were no problem. See the discussion for pull request #2422 which was the first one to reduce the size of tesscallback.h.
563a171
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason to reduce tesscallback.h is that's a very large header file (more than 350 KB!) without any direct use for API users. More than 9000 lines of undocumented code are not something which must be preserved for API compatibility. Binary compatibility needs separate consideration.
563a171
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zdenop, if you can reproduce the API check locally, a short description might help me to get the same results. Up to now, my local check does not indicate API problems.
563a171
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the description. I think I could fix the binary ABI compatibility now, see report.