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

add a driver manager wrapper #324

Merged
merged 5 commits into from Oct 31, 2022
Merged

add a driver manager wrapper #324

merged 5 commits into from Oct 31, 2022

Conversation

ChristianBeilschmidt
Copy link
Contributor

@ChristianBeilschmidt ChristianBeilschmidt commented Oct 27, 2022

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

I added a wrapper for the DriverManager.
I moved some methods to this struct's impl and deprecated them in the Driver.
Moreover, I created integration tests that use the manager. They would interfere with other tests.

@metasim
Copy link
Contributor

metasim commented Oct 27, 2022

@ChristianBeilschmidt It's interesting that we both were doing work on this at the same time (but I think for different reasons). Here's what I was doing:

https://github.com/georust/gdal/pull/323/files

Why do you think DriverManager is necessary over just extending Driver?

Copy link
Contributor

@metasim metasim left a comment

Choose a reason for hiding this comment

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

I'm interested to know why we need to move the Driver methods into DriverManager, creating deprecation issues for others. Did you consider just putting the enhanced registration capabilities in DriverManager and leaving the Driver public API as it is?

src/driver.rs Outdated
///
/// ```rust, no_run
/// use gdal::Driver;
/// println!("{} drivers are registered", Driver::count());
Copy link
Contributor

Choose a reason for hiding this comment

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

If Driver is to be deprecated, should these examples be updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ChristianBeilschmidt
Copy link
Contributor Author

I'm interested to know why we need to move the Driver methods into DriverManager, creating deprecation issues for others. Did you consider just putting the enhanced registration capabilities in DriverManager and leaving the Driver public API as it is?

As far as I see GDAL's C++-API, it is structured as I did above. It differentiates between DriverManager and Driver. So it makes sense from my point of view to put these methods (previously in Driver) there.

@metasim
Copy link
Contributor

metasim commented Oct 27, 2022

@ChristianBeilschmidt Thanks for explaining the correspondence with the C++ implementation. I wasn't aware of that.

@metasim metasim requested a review from lnicola October 27, 2022 18:18
@jdroenner
Copy link
Member

it looks good to me

pub fn get_driver(index: usize) -> Result<Driver> {
_register_drivers();
let c_driver = unsafe { gdal_sys::GDALGetDriver(index.try_into().unwrap()) };
if c_driver.is_null() {
Copy link
Member

@lnicola lnicola Oct 28, 2022

Choose a reason for hiding this comment

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

I don't think GDALGetDriver sets the last error: https://github.com/OSGeo/gdal/blob/master/gcore/gdal_priv.h#L1734. _last_null_pointer_err might return a bogus error here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/driver.rs Outdated
let c_name = CString::new(name)?;
let c_driver = unsafe { gdal_sys::GDALGetDriverByName(c_name.as_ptr()) };
if c_driver.is_null() {
return Err(_last_null_pointer_err("GDALGetDriverByName"));
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

///
/// Wraps [`GDALRegisterDriver()`](https://gdal.org/api/raster_c_api.html#_CPPv418GDALRegisterDriver11GDALDriverH)
pub fn register_driver(driver: &Driver) -> usize {
let index = unsafe { gdal_sys::GDALRegisterDriver(driver.c_driver) };
Copy link
Member

@lnicola lnicola Oct 28, 2022

Choose a reason for hiding this comment

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

It's not impossible, but it's pretty hard to end up needing to call this (because we do _register_drivers in Get*). But we should have it, of course.


/// Prevents the automatic registration of all known GDAL drivers when first calling create, open, etc.
pub fn prevent_auto_registration() {
START.call_once(|| {});
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@lnicola
Copy link
Member

lnicola commented Oct 28, 2022

bors d+

@bors
Copy link
Contributor

bors bot commented Oct 28, 2022

✌️ ChristianBeilschmidt can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@jdroenner
Copy link
Member

bors merge

@bors
Copy link
Contributor

bors bot commented Oct 31, 2022

Build succeeded:

@bors bors bot merged commit 2ba3db2 into master Oct 31, 2022
@ChristianBeilschmidt ChristianBeilschmidt deleted the dataset-removal branch October 31, 2022 11:46
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

4 participants