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

Isolate solv::ObjQueue #2289

Merged
merged 10 commits into from
Mar 2, 2023
Merged

Isolate solv::ObjQueue #2289

merged 10 commits into from
Mar 2, 2023

Conversation

AntoinePrv
Copy link
Member

@AntoinePrv AntoinePrv commented Feb 14, 2023

Part of #2302.

Here the Queue is isolated in a safe RAII object.
It was made to use std conventions rather than libsolv ones.

This is private API so does not have any consequence.

On the immediate side, it caught some memory leaks in MSolver!

@AntoinePrv AntoinePrv self-assigned this Feb 14, 2023
@AntoinePrv AntoinePrv force-pushed the solv branch 2 times, most recently from 2278171 to 9857d33 Compare February 16, 2023 10:17
@AntoinePrv AntoinePrv requested review from Klaim and Hind-M February 16, 2023 17:37
Copy link
Member

@Hind-M Hind-M left a comment

Choose a reason for hiding this comment

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

Nice!
A slight nitpicking, new files should have 2023 instead of 2019 or 2022 in the license section for history consistency.

@AntoinePrv AntoinePrv requested a review from wolfv February 20, 2023 07:46
libmamba/src/core/solver.cpp Outdated Show resolved Hide resolved
@wolfv
Copy link
Member

wolfv commented Feb 20, 2023

Looks very good to me!

I was wondering about the header location (solv-cpp/queue.hpp vs something like mamba/internal/solv-cpp/queue.hpp). But I guess it doesn't matter much since we're not installing that header, right?

And the rest was just some tiny comments.

@AntoinePrv AntoinePrv requested a review from wolfv February 20, 2023 10:36
@AntoinePrv
Copy link
Member Author

@wolfv Yes these headers are private (hence the std::unique_ptr).
On the longer run we could make it its own implementation library, that is in CMake but never packaged or installed.
That would further ensure that we are keeping a strict separation of abstractions.

@@ -252,6 +253,10 @@ set(LIBMAMBA_HEADERS
${LIBMAMBA_INCLUDE_DIR}/mamba/api/update.hpp
)

set(LIBMAMBA_PRIVATE_HEADERS
${LIBMAMBA_SOURCE_DIR}/solv-cpp/queue.hpp
Copy link
Member

Choose a reason for hiding this comment

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

Note that technically libmamba have more private headers, notably the ones in src/core/

Copy link
Member Author

Choose a reason for hiding this comment

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

They are not really private, they are installed, and needed by public headers.

libmamba/include/mamba/core/package_info.hpp Show resolved Hide resolved
libmamba/src/core/package_info.cpp Outdated Show resolved Hide resolved
libmamba/src/solv-cpp/queue.hpp Outdated Show resolved Hide resolved
private:
friend void swap(ObjQueue& a, ObjQueue& b) noexcept;

::Queue m_queue = {};
Copy link
Member

Choose a reason for hiding this comment

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

Side note that stylistically I would prefer default initialization to not have an assignation. I understand that it's valid in C++17, but it looks redundant and noisy to me when reading the code.
Weak "meh" here, to be clear.

libmamba/src/solv-cpp/queue.hpp Outdated Show resolved Hide resolved
libmamba/src/solv-cpp/queue.cpp Outdated Show resolved Hide resolved
libmamba/src/solv-cpp/queue.cpp Show resolved Hide resolved
Comment on lines +163 to +164
std::stringstream ss = {};
ss << "Index " << pos << " is greater that the number of elements (" << size << ')';
Copy link
Member

Choose a reason for hiding this comment

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

Use fmt here (maybe with a comment to use std::format once C++20 is available)

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea is to keep these wrappers dependency free so I think we should avoid using fmt until there is some strong need for it.

@AntoinePrv AntoinePrv closed this Feb 27, 2023
@AntoinePrv AntoinePrv deleted the solv branch February 27, 2023 14:41
@AntoinePrv AntoinePrv restored the solv branch February 27, 2023 16:15
@AntoinePrv
Copy link
Member Author

Bad branch manipulation.

@AntoinePrv AntoinePrv reopened this Feb 27, 2023
@JohanMabille JohanMabille merged commit 24261d1 into mamba-org:main Mar 2, 2023
@AntoinePrv AntoinePrv deleted the solv branch March 2, 2023 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants