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 strcpy and strncpy by new inline helper function #4250

Merged
merged 1 commit into from
May 24, 2024

Conversation

stweil
Copy link
Contributor

@stweil stweil commented May 24, 2024

No description provided.

@egorpugin
Copy link
Contributor

egorpugin commented May 24, 2024

We should name it copy_string__remove_after_transition_to_std_strings().

@@ -310,11 +308,7 @@ char *LTRResultIterator::WordTruthUTF8Text() const {
if (!HasTruthString()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we change this function return type to std::string?

char *LTRResultIterator::WordTruthUTF8Text() const {

to

std::string LTRResultIterator::WordTruthUTF8Text() const {

and also all calling sites?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even better, WordTruthUTF8Text should be removed because it is not used anywhere. But that can be done in a separate commit.

That function is part of the public API, so I think we have to keep it as it is.

@stweil
Copy link
Contributor Author

stweil commented May 24, 2024

We should name it copy_string__remove_after_transition_to_std_strings().

That's a little bit long, but a comment which basically says the same thing could be added in helpers.h.

@@ -310,11 +308,7 @@ char *LTRResultIterator::WordTruthUTF8Text() const {
if (!HasTruthString()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even better, WordTruthUTF8Text should be removed because it is not used anywhere. But that can be done in a separate commit.

That function is part of the public API, so I think we have to keep it as it is.

@@ -35,6 +35,15 @@

namespace tesseract {

// Copy a std::string to a newly allocated char *.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
// Copy a std::string to a newly allocated char *.
// Copy a std::string to a newly allocated char *.
// TODO: Remove this function once the related code has been converted to use std::string.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
@stweil stweil merged commit c5b0c2f into tesseract-ocr:main May 24, 2024
5 of 6 checks passed
@stweil stweil deleted the strcpy branch May 24, 2024 13:24
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.

None yet

2 participants