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

Replace alloc_struct, free_struct #744

Merged
merged 1 commit into from
Apr 30, 2017
Merged

Conversation

stweil
Copy link
Contributor

@stweil stweil commented Mar 2, 2017

Both functions simply call malloc, free.

Remove also unneeded null pointer checks and use calloc where possible.

Signed-off-by: Stefan Weil [email protected]

@amitdo
Copy link
Collaborator

amitdo commented Mar 2, 2017

see #11

@amitdo
Copy link
Collaborator

amitdo commented Mar 3, 2017

Also. it seems that this PR only changes parts of the old engine that Ray wants to drop.

@stweil
Copy link
Contributor Author

stweil commented Mar 6, 2017

@amitdo, do you think that the PR should not be applied because it changes code which might be removed later?

@amitdo
Copy link
Collaborator

amitdo commented Mar 6, 2017

@theraysmith commented in #518

Please provide examples of where you get better results with the old engine.
Right now I'm trying to work on getting rid of redundant code, rather than spending time fighting needless changes that generate a lot of work.

Instead of modifying the old engine parts, concentrate on convincing me why it should stay first

To me, the message here from Ray is clear:
<<That's NOT an actual quote of Ray>>
"I don't want you* to do any change to the old engine. Doing so just interfere with my work on the new engine".

That's the way I understand it.

* 'you' - not just you, any developer.

see #11

#11 (review)

@amitdo
Copy link
Collaborator

amitdo commented Mar 6, 2017

Here is the way I believe things are working in the Google side.

Every commit that we push to this repo, at some time in the future (few weeks/months) will be reviewed by Ray or someone else from Google. If they don't like something they will change it. Then it will be merged with the internal Google branch, which might diverge significantly from the public repo's master branch. I think they do it infrequently, which means that when they try to merge with us they have quite a lot of work to do and it is not so simple as you might think.

@theraysmith, @jbreiden
You are invited to correct me if I'm wrong here.

@theraysmith
Copy link
Contributor

@amitdo, a fair assessment.
Although I haven't made any major changes to the codebase in the last few weeks, yes I would still like to remove the old classifier and take out a lot of code with it.

I'm going to review the replies to my request for "old better than new", and thanks to those that provided them, with a view to making new better than old on those problems. I haven't got to that yet, as I have been totally occupied by fixing many problems I have found with the synthetic training data generation pipeline. Since training takes about 2 weeks, fixes to the source data get priority treatment over other code changes.

Yes, each commit gets reviewed by me and someone else at Google.

@amitdo
Copy link
Collaborator

amitdo commented Apr 15, 2017

See also:
#752 (comment)

Both functions simply call malloc, free.

Remove also unneeded null pointer checks and use calloc where possible.

Signed-off-by: Stefan Weil <[email protected]>
@stweil
Copy link
Contributor Author

stweil commented Apr 30, 2017

I rebased the code to fix some merge conflicts.

Ray, I noticed your recent changes which introduced TFile in those files. IMO removing memry.{cpp,h} would also be a good step to clean the code. I‌ could do that in two steps to simplify reviews if you agree:

  1. switch to using malloc / calloc and free in a first step (like it is done in this PR)
  2. replace all those allocations by C++ code (new / new[] and delete / delete[])

@zdenop zdenop merged commit eaf5629 into tesseract-ocr:master Apr 30, 2017
@amitdo
Copy link
Collaborator

amitdo commented Apr 30, 2017

replace all those allocations by C++ code (new / new[] and delete / delete[])

Isn't this remark from Ray also relevant here?
#11 (review)

@stweil
Copy link
Contributor Author

stweil commented Apr 30, 2017

It isn't, as I promised to use C++ code in step 2 (see above).

@amitdo
Copy link
Collaborator

amitdo commented Apr 30, 2017

How about replacing the underlying pointer variables with std::array, thus hiding all calls to allocate and delete the memory?

If you know what he meant, consider implementing it.

@stweil stweil deleted the malloc branch May 1, 2017 14:44
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.

4 participants