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

OGRGeometry: Add BufferEx method #9924

Merged
merged 1 commit into from
May 16, 2024
Merged

Conversation

dbaston
Copy link
Member

@dbaston dbaston commented May 14, 2024

What does this PR do?

Adds an OGRGeometry::BufferEx method to allow passing key/value options.

What are related issues/pull requests?

#8132

Tasklist

  • Add test case(s)
  • Add documentation
  • Updated Python API documentation (swig/include/python/docs/)
  • Review
  • Adjust for comments
  • All CI builds and checks have passed

@dbaston
Copy link
Member Author

dbaston commented May 14, 2024

I'm not sure of the cleanest way to handle this. I guess I could expose BufferEx under a separate name. The tests pass despite the warning.

/home/dan/dev/gdal/swig/include/ogr.i:3713: Warning 511: Can't use keyword arguments with overloaded functions (OGRGeometryShadow::Buffer(double,int)).
/home/dan/dev/gdal/swig/include/ogr.i:3717: Warning 511: Can't use keyword arguments with overloaded functions (OGRGeometryShadow::Buffer(double,char **)).

try
{
std::size_t end;
int nQuadSegs = std::stoi(pszValue, &end, 10);
Copy link
Member Author

Choose a reason for hiding this comment

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

This parameter parsing/validation is really verbose and makes me wonder if I'm missing something like std::optional<int> CPLReadInt(std::string_view) or bool CPLReadInt(const char*, int*).

std::from_chars is the best solution I've found but I recently ran into problems with it being unsupported on recent versions of Apple clang.

Copy link
Member

Choose a reason for hiding this comment

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

std::from_chars is the best solution I've found but I recently ran into problems with it being unsupported on recent versions of Apple clang.

well, it is now a requirement since it is used in apps/argparse/argparse.hpp

Otherwise, I wouldn't care much about validating that the provided string is in range and would just blindly use atoi() . We do that almost everywhere....

@rouault
Copy link
Member

rouault commented May 14, 2024

I'm not sure of the cleanest way to handle this. I guess I could expose BufferEx under a separate name. The tests pass despite the warning.

yeah, we have a similar issue with Feature::SetField(). One solution might be, for Python, to expose different SWIG method names for the 2 forms of Buffer, and have a Python method in swig/include/python/ogr_python.i that redirects to the appropriate one.

ogr/ogrgeometry.cpp Outdated Show resolved Hide resolved
try
{
std::size_t end;
int nQuadSegs = std::stoi(pszValue, &end, 10);
Copy link
Member

Choose a reason for hiding this comment

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

std::from_chars is the best solution I've found but I recently ran into problems with it being unsupported on recent versions of Apple clang.

well, it is now a requirement since it is used in apps/argparse/argparse.hpp

Otherwise, I wouldn't care much about validating that the provided string is in range and would just blindly use atoi() . We do that almost everywhere....

ogr/ogrgeometry.cpp Show resolved Hide resolved
ogr/ogrgeometry.cpp Show resolved Hide resolved
@coveralls
Copy link
Collaborator

coveralls commented May 15, 2024

Coverage Status

coverage: 69.103% (-0.009%) from 69.112%
when pulling f49da24 on dbaston:buffer-params
into 0367605 on OSGeo:master.

@dbaston
Copy link
Member Author

dbaston commented May 16, 2024

One solution might be, for Python, to expose different SWIG method names for the 2 forms of Buffer, and have a Python method in swig/include/python/ogr_python.i that redirects to the appropriate one.

I guess the alternative is to disable kwargs for this function, which breaks the usage

ogr.CreateGeometryFromWkt("POINT (0 0)").Buffer(1, quadsecs=1)

That doesn't bother me, especially with "quadsecs" being a typo, but I'm fine to go either way.

@rouault
Copy link
Member

rouault commented May 16, 2024

is to disable kwargs [... ] That doesn't bother me, especially with "quadsecs" being a typo,

That would be fine too

@rouault rouault merged commit 3632b72 into OSGeo:master May 16, 2024
35 checks passed
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.

3 participants