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

Adapt to new api introduced in kiwix/libkiwix#991 #633

Merged
merged 3 commits into from Oct 12, 2023

Conversation

mgautierfr
Copy link
Member

@mgautierfr mgautierfr commented Aug 23, 2023

See kiwix/libkiwix#991 for more details

src/manager/kiwix-manage.cpp Outdated Show resolved Hide resolved
}
}
return(0);
}

int handle_add(kiwix::Library* library, const std::string& libraryPath,
int handle_add(kiwix::LibraryPtr library, const std::string& libraryPath,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that a parameter of type kiwix::Library& is more suitable here (passing a shared_ptr suggests that it is preserved somewhere and can be accessed after the function returns).

Copy link
Member Author

Choose a reason for hiding this comment

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

We pass the library to the Manager which take a LibraryPtr.
So I have keep LibraryPtr instead of kiwix::Library&.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the Manager object is completely internal to handle_add() (it no longer exists when the function returns) it would be perfectly safe to convert the Library& into a non-owning shared_ptr inside the function.

src/manager/kiwix-manage.cpp Outdated Show resolved Hide resolved
@mgautierfr mgautierfr force-pushed the adapt_libkiwix_991 branch 3 times, most recently from 13d8333 to 61055ef Compare October 6, 2023 12:46
@kelson42
Copy link
Contributor

kelson42 commented Oct 8, 2023

Would be good to merge soon, it is breaking CI of many projects.

@mgautierfr
Copy link
Member Author

The two first commits are not especially related to kiwix/likiwix#991 but it fix the CI and without them, no PR pass.

It was simpler to make on PR than two PR and merge one that was broken, expecting that the other would be fixed after.

@mgautierfr
Copy link
Member Author

This PR fixes the CI which is broken since several days now.
I'm merging it. @veloman-yunkan, if you still have comments on it, please open a issue.

@mgautierfr mgautierfr merged commit 3a0e87d into main Oct 12, 2023
10 checks passed
@mgautierfr mgautierfr deleted the adapt_libkiwix_991 branch October 12, 2023 16:05
@veloman-yunkan
Copy link
Collaborator

This PR fixes the CI which is broken since several days now. I'm merging it. @veloman-yunkan, if you still have comments on it, please open a issue.

@mgautierfr The only comment that I had doesn't deserve an issue.

@kelson42 kelson42 added this to the 3.6.0 milestone Nov 19, 2023
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

3 participants