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

refactor: move misplaced TF_LITE_REMOVE_VIRTUAL_DELETEs to private: #2980

Closed
wants to merge 1 commit into from

Conversation

rkuester
Copy link
Contributor

@rkuester rkuester commented Dec 2, 2024

Move several TF_LITE_REMOVE_VIRTUAL_DELETE declarations that are
wrongly in a public section of their classes. To have the intended
effect, as documented in t/l/m/compatibility.h, these must be in a
private section.

BUG=see description

Move several TF_LITE_REMOVE_VIRTUAL_DELETE declarations that are
wrongly in a public section of their classes. To have the intended
effect, as documented in t/l/m/compatibility.h, these must be in a
private section.
@rkuester rkuester requested a review from a team as a code owner December 2, 2024 20:19
@suleshahid
Copy link
Collaborator

Seems like these were moved out of private intentionally? e814105

@rkuester
Copy link
Contributor Author

rkuester commented Dec 5, 2024

If the right fix for whatever that problem was is to make the destructors public: accessible, it certainly breaks the intent, naming, and comments relating to the define TF_LITE_REMOVE_VIRTUAL_DELETE. It might be a sign those classes are called in unintended ways. If the destructors are indeed desired, they should probably be defined without the macro.

This commit is actually from quite a while ago, and I've forgotten now whether I had a need to fix it, or whether I just spotted the inconsistency and cleaned up. I'll drop this PR for now, and resurrect it if this becomes necessary for the compression feature #2636.

@rkuester rkuester closed this Dec 5, 2024
@rkuester rkuester deleted the fix-misplaced-deletes branch December 6, 2024 23:07
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.

3 participants