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

Shared pointer should be used where appropriate #988

Closed
kelson42 opened this issue Aug 11, 2023 · 3 comments · Fixed by #991
Closed

Shared pointer should be used where appropriate #988

kelson42 opened this issue Aug 11, 2023 · 3 comments · Fixed by #991
Assignees
Milestone

Comments

@kelson42
Copy link
Collaborator

See kiwix/kiwix-android#3467

@kelson42 kelson42 added this to the 13.0.0 milestone Aug 11, 2023
@kelson42 kelson42 changed the title Share pointer should be used where appropriate Shared pointer should be used where appropriate Aug 11, 2023
@kelson42
Copy link
Collaborator Author

@veloman-yunkan @mgautierfr Reading #991 (review), it seems to me there is a discrepency between both of you on (1) the nature of the problem (2) the path to follow to improve the situation. I have no optinion at all, but I would prefer to have you reaching a kind of of agreement on these two points before continuing PR implementation.

@mgautierfr
Copy link
Member

Here a proposition:

  • Move html_dumper, libxml_dumper and opds_dumper behind a free function (implementation will not be changed, but classes will not be part of the public api).
  • Make server take a shared_ptr to library and namemapper.
  • Keep the proposition in Use std::shared_ptr instead of raw pointer. #991 to create the library only on the heap through a shared_ptr (?)
  • If previous point is not done, introduce a free function as proposed here to be able to create a not owning shared ptr (internal only ?) for case when user want to create a a server on library

@veloman-yunkan
Copy link
Collaborator

veloman-yunkan commented Sep 3, 2023

Here a proposition:

  • Move html_dumper, libxml_dumper and opds_dumper behind a free function (implementation will not be changed, but classes will not be part of the public api).

  • Make server take a shared_ptr to library and namemapper.

  • Keep the proposition in Use std::shared_ptr instead of raw pointer. #991 to create the library only on the heap through a shared_ptr (?)

  • If previous point is not done, introduce a free function as proposed here to be able to create a not owning shared ptr (internal only ?) for case when user want to create a a server on library

I am fine with that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants