-
Notifications
You must be signed in to change notification settings - Fork 543
[IMPROVEMENT] Remove useless O(N) operations and memory allocations #1207
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
Conversation
|
CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results:
It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). Your PR breaks these cases:
Check the result page for more info. |
|
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results:
It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). Your PR breaks these cases:
Check the result page for more info. |
src/lib_ccx/ocr.c
Outdated
| ccx_options.write_format==CCX_OF_WEBVTT) | ||
| { | ||
| char *substr; | ||
| char substr[sizeof("</font><font color=\"#000000\">")]; |
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.
A hardcoded string that appears more than once definitely belongs in a #define - otherwise a change in one of the instances will break stuff (particularly if sizeof is involved)
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.
It only appears once now.
I also managed to remove another O(N) operation. (copying sprintf output).
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.
We're processing streams, O(N) is the best we can possibly ask for :-) Don't obsess over micro-optimizations. If you see O (N^2) then that's something to worry about. But a 10 character memcpy() is not worth the time, even if it can be avoided.
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.
yeah I know 😂
it's not gonna change a lot.
O(N) is the best we can possibly ask for
yes but it's best to not copy stuff around when you don't have to
863250c to
98b7315
Compare
In raising this pull request, I confirm the following (please check boxes):
My familiarity with the project is as follows (check one):
And move some processing to compile time. It also uses functions that don't check needlessly against '\0'.