OGRGeometryFactory: Add createFromWkt overload returning unique_ptr

[dbaston]

What does this PR do?

Adds an overload of OGRGeometry::createFromWkt to more easily get a unique_ptr<OGRGeometry> from a WKT string. I don't know how others feel about the std::pair return in place of an "out" parameter. The diff in autotest/cpp/test_ogr.cppautotest/cpp/test_ogr.cpp demonstrates the simplification nicely.

Unfortunately this technique isn't generally applicable to methods currently returning a raw pointer. I'm not sure what the best pattern for addressing those is. Maybe just adding a new method with a U at the end? For example, std::unique_ptr<OGRPolygon> OGRGeometryFactory::forceToPolygonU(const OGRGeometry*). Ugly, but I haven't thought of a better idea yet.

[rouault]

Maybe just adding a new method with a U at the end? For example, std::unique_ptr<OGRPolygon> OGRGeometryFactory::forceToPolygonU(const OGRGeometry*)

As forceToPolygon() consumes the input geometry, std::unique_ptr<OGRPolygon> OGRGeometryFactory::forceToPolygonU(std::unique_ptr<OGRGeometry>) shouldn't conflict with the existing method.
Also note that forceToPolygon() might not return a polygon if it fails, but the original geometry, so the return type should be std::unique_ptr<OGRGeometry> as well. Unless you plan to implement a more restrictive behavior.

In the more general case, if we don't want to break backwards compatibility, AsUniquePtr could also be a potential verbose suffix.

[coveralls]

coverage: 69.422% (+0.001%) from 69.421%
when pulling 886fe26 on dbaston:ogrgf-fromwkt-return-unique
into b4a3fde on OSGeo:master.

[rouault]

Unfortunately this technique isn't generally applicable to methods currently returning a raw pointer. I'm not sure what the best pattern for addressing those is. Maybe just adding a new method with a U at the end?

Maybe @elpaso has some thoughts about that topic of how to keep methods that return a raw pointer vs enhanced ones that would return a unique_ptr ?

[elpaso]

Unfortunately this technique isn't generally applicable to methods currently returning a raw pointer. I'm not sure what the best pattern for addressing those is. Maybe just adding a new method with a U at the end?

Maybe @elpaso has some thoughts about that topic of how to keep methods that return a raw pointer vs enhanced ones that would return a unique_ptr ?

Not sure: can you point me to a method that should be converted to std::unique_ptr (besides forceToPolygon that if I get this right doesn't have a problem)?.

[dbaston]

can you point me to a method that should be converted to std::unique_ptr

OGRGeometry::Buffer, OGRGeometry::Intersection, OGRFeature::StealGeometry, etc.

[elpaso]

can you point me to a method that should be converted to std::unique_ptr

OGRGeometry::Buffer, OGRGeometry::Intersection, OGRFeature::StealGeometry, etc.

I don't see many options here, we could create a set of new methods with a different name/suffix (something like AsUnique) and deprecate the old methods (or not).

Regarding methods that might return the original geometry instead of a new one, I would probably change their behavior to throw an exception or return a null (unique) ptr.

[rouault]

I would probably change their behavior to throw an exception

The GDAL C++ API doesn't throw exceptions. Most of the code isn't ready for that given there are manual C style memory allocations, and that would cause memory leaks. C++ exceptions also apparently tend to cause issues when people run under debugger on Windows I believe (at least users seem to be regularly annoyed by PROJ throwing occasionaly C++ exceptions for its internal needs, despite PROJ catching them before they face the caller)

[dbaston]

An alternative to a return type of std::pair<std::unique_ptr<X>, OGRErr> would be a std::variant<std::unique_ptr<X>, OGRErr>. That's in some ways more explicit (you either get the thing you asked for, or an error explaining why you didn't). But it would be quite a bit messier at the call site.

Instead of

auto [poGeom, err] = fn();

you'd have

auto vGeom = fn();
if (std::holds_alternative<OGRErr>(vGeom)) {
  // handle error
}
auto poGeom = std::get<std::unique_ptr<OGRGeometry>>(vGeom);

I prefer the std::pair option. I don't work on other C++ projects that don't use exceptions, so I'm not sure if there are other conventions out there.

[rouault]

I prefer the std::pair option.

yes, for now, we've restricted use of C++17 features to our GDAL internal use. https://gdal.org/en/latest/development/rfc/rfc98_build_requirements_gdal_3_9.html : "While we want to allow C++17 features to build GDAL, for now, we will stick to exposing at most C++11 features in the exported headers of the library to minimize disruption for GDAL C++ users. That might be revisited later" .