Proper deallocation of strings in the constructor #216
Conversation
This file contains hidden or 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
Contributor
|
@serizba Sorry for my late response. The code looks good to me. I also tested it on Linux with libtensorflow 2.5, and it worked. We probably need to tell people loudly that pre-2.4 versions (e.g. 2.3) do not work anymore. |
Owner
Author
|
I will do that then. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently there isn't support for TF_TString tensors. Besides, they require a proper dealloaction with
TF_TString_Dealloc.This has been mentioned in #196 and #200. A solution was proposed in #128 but it didn't work.
I proposed a solution in #200, but I would like some feedback before merging in. This PR includes that code.
I only included a specialized constructor for vectors of strings. Constructor with single strings and with initializer lists will call this constructor, so no need to specialize them.
I also removed the old code (before TF 2.4) way of using strings.
@ljn917 what do you think about this constructor? should we merge?