-
Notifications
You must be signed in to change notification settings - Fork 11.6k
clip : use smart pointer (⚠️ breaking change) #12869
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
Conversation
examples/llava/clip-impl.h
Outdated
struct clip_image_u8_ptr : std::unique_ptr<clip_image_u8, clip_image_u8_deleter> { | ||
clip_image_u8_ptr() : std::unique_ptr<clip_image_u8, clip_image_u8_deleter>(clip_image_u8_init()) {} | ||
}; | ||
|
||
struct clip_image_f32_ptr : std::unique_ptr<clip_image_f32, clip_image_f32_deleter> { | ||
clip_image_f32_ptr() : std::unique_ptr<clip_image_f32, clip_image_f32_deleter>(clip_image_f32_init()) {} | ||
}; | ||
|
||
typedef std::unique_ptr<clip_image_size, clip_image_size_deleter> clip_image_size_ptr; | ||
|
||
// these need to be struct to maintain compatibility with C interface | ||
struct clip_image_u8_batch : std::vector<clip_image_u8_ptr> { | ||
clip_image_u8_batch() : std::vector<clip_image_u8_ptr>() {} | ||
}; | ||
struct clip_image_f32_batch : std::vector<clip_image_f32_ptr> { | ||
clip_image_f32_batch() : std::vector<clip_image_f32_ptr>() {} | ||
~clip_image_f32_batch() {} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inheritance from STL classes is generally not recommended. Although it's tempting, I think it would be better to avoid establishing this practice, as it can lead to subtle issues.
The recommended way is to use composition instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that's good to know! (And I will also need to do the same on my llama_batch_ext
PR)
With the help of gemini 3.5 pro, I added the composite in f8c5394
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about something much simpler like:
struct clip_image_u8_batch {
std::vector<clip_image_u8_ptr> entries;
};
// ...
// end then using directly the `entries` member in the code
batch.entries.clear();
batch.entries.push_back(...);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes thanks, I simplify this in f727124
The clip_image_u8_ptr
is also no longer being initialized by default, I think it's not a big deal, reduce the complexity of the wrapper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(And I will also need to do the same on my llama_batch_ext PR)
Yup, I will get back to this PR after some refactoring in libllama
. We had a discussion with @slaren how to improve on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that's good to know! (And I will also need to do the same on my
llama_batch_ext
PR)With the help of gemini 3.5 pro, I added the composite in f8c5394
Inheriting from std::unique_ptr
is very strange usage of C++ pointers.
Also, std::shared_ptr
might be a better option than std::unique_ptr
as the former avoids dangling references. The overhead is usually not significant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh unique_ptr
gives me more confident when writing code, kinda rust-like experience
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh
unique_ptr
gives me more confident when writing code, kinda rust-like experience
No. They are not equivalent to rust semantics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I said rust-like experience, not rust-equivalent semantics. unique_ptr is useful in my case because the compiler will complain about unique_ptr being non-copiable, so I have total control over the life cycle of the object owned by unique_ptr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Just be aware of
.get() usage as it could create dangling pointers despite unique ownership.
* clip : use smart pointers * fix warmup * add forward declaration * misisng include * fix include (2) * composite * simplify batch ptr * fix conflict
Summarize this change
Breaking change to clip.cpp
The
struct clip_image_f32_batch
now become an opaque pointer. To access the underlay data, use these API calls:Note: The
struct clip_image_u8_batch
is also became an opaque pointer, but I cannot find any code actually using it. If this change affect you, please leave a comment in this PR.Note 2: If your existing code initialize
clip_image_f32_batch
on heap, here is how to migrate:Test