From 1c459f57d9409f9cffa35cd619957d7c5e9163bf Mon Sep 17 00:00:00 2001 From: Daniel Baston Date: Mon, 28 Oct 2024 11:00:41 -0400 Subject: [PATCH] OGRFeature: Add SetGeometry overloads taking unique_ptr --- autotest/cpp/test_ogr.cpp | 63 ++++++++++++++++++++ ogr/ogr_feature.h | 2 + ogr/ogrfeature.cpp | 117 +++++++++++++++++++++++++++++--------- 3 files changed, 155 insertions(+), 27 deletions(-) diff --git a/autotest/cpp/test_ogr.cpp b/autotest/cpp/test_ogr.cpp index 5ae28fcd95f7..9f2de42a3150 100644 --- a/autotest/cpp/test_ogr.cpp +++ b/autotest/cpp/test_ogr.cpp @@ -4394,4 +4394,67 @@ TEST_F(test_ogr, OGRPolygon_addRingDirectly) ASSERT_EQ(p.addRingDirectly(lr.release()), OGRERR_NONE); } +TEST_F(test_ogr, OGRFeature_SetGeometry) +{ + OGRFeatureDefn *poFeatureDefn = new OGRFeatureDefn(); + poFeatureDefn->Reference(); + + OGRFeature oFeat(poFeatureDefn); + std::unique_ptr poGeom; + OGRGeometry *poTmpGeom; + ASSERT_EQ( + OGRGeometryFactory::createFromWkt("POINT (3 7)", nullptr, &poTmpGeom), + OGRERR_NONE); + poGeom.reset(poTmpGeom); + ASSERT_EQ(oFeat.SetGeometry(std::move(poGeom)), OGRERR_NONE); + EXPECT_EQ(oFeat.GetGeometryRef()->toPoint()->getX(), 3); + EXPECT_EQ(oFeat.GetGeometryRef()->toPoint()->getY(), 7); + + // set it again to make sure previous feature geometry is freed + ASSERT_EQ( + OGRGeometryFactory::createFromWkt("POINT (2 8)", nullptr, &poTmpGeom), + OGRERR_NONE); + poGeom.reset(poTmpGeom); + ASSERT_EQ(oFeat.SetGeometry(std::move(poGeom)), OGRERR_NONE); + EXPECT_EQ(oFeat.GetGeometryRef()->toPoint()->getX(), 2); + EXPECT_EQ(oFeat.GetGeometryRef()->toPoint()->getY(), 8); + + poFeatureDefn->Release(); +} + +TEST_F(test_ogr, OGRFeature_SetGeomField) +{ + OGRFeatureDefn *poFeatureDefn = new OGRFeatureDefn(); + poFeatureDefn->Reference(); + + OGRGeomFieldDefn oGeomField("second", wkbPoint); + poFeatureDefn->AddGeomFieldDefn(&oGeomField); + + OGRFeature oFeat(poFeatureDefn); + + // failure + { + std::unique_ptr poGeom; + OGRGeometry *poTmpGeom; + ASSERT_EQ(OGRGeometryFactory::createFromWkt("POINT (3 7)", nullptr, + &poTmpGeom), + OGRERR_NONE); + poGeom.reset(poTmpGeom); + EXPECT_EQ(oFeat.SetGeomField(13, std::move(poGeom)), OGRERR_FAILURE); + } + + // success + { + std::unique_ptr poGeom; + OGRGeometry *poTmpGeom; + ASSERT_EQ(OGRGeometryFactory::createFromWkt("POINT (3 7)", nullptr, + &poTmpGeom), + OGRERR_NONE); + poGeom.reset(poTmpGeom); + EXPECT_EQ(oFeat.SetGeomField(1, std::move(poGeom)), OGRERR_NONE); + } + + poFeatureDefn->Release(); +} + } // namespace diff --git a/ogr/ogr_feature.h b/ogr/ogr_feature.h index b19421f6fdb7..d710dfd54a93 100644 --- a/ogr/ogr_feature.h +++ b/ogr/ogr_feature.h @@ -1178,6 +1178,7 @@ class CPL_DLL OGRFeature OGRErr SetGeometryDirectly(OGRGeometry *); OGRErr SetGeometry(const OGRGeometry *); + OGRErr SetGeometry(std::unique_ptr); OGRGeometry *GetGeometryRef(); const OGRGeometry *GetGeometryRef() const; OGRGeometry *StealGeometry() CPL_WARN_UNUSED_RESULT; @@ -1209,6 +1210,7 @@ class CPL_DLL OGRFeature const OGRGeometry *GetGeomFieldRef(const char *pszFName) const; OGRErr SetGeomFieldDirectly(int iField, OGRGeometry *); OGRErr SetGeomField(int iField, const OGRGeometry *); + OGRErr SetGeomField(int iField, std::unique_ptr); void Reset(); diff --git a/ogr/ogrfeature.cpp b/ogr/ogrfeature.cpp index 426e9b398600..1cc4f594dd44 100644 --- a/ogr/ogrfeature.cpp +++ b/ogr/ogrfeature.cpp @@ -433,7 +433,7 @@ OGRFeatureDefnH OGR_F_GetDefnRef(OGRFeatureH hFeat) /** * \brief Set feature geometry. * - * This method updates the features geometry, and operate exactly as + * This method updates the features geometry, and operates the same as * SetGeometry(), except that this method assumes ownership of the * passed geometry (even in case of failure of that function). * @@ -456,11 +456,12 @@ OGRFeatureDefnH OGR_F_GetDefnRef(OGRFeatureH hFeat) OGRErr OGRFeature::SetGeometryDirectly(OGRGeometry *poGeomIn) { - if (GetGeomFieldCount() > 0) - return SetGeomFieldDirectly(0, poGeomIn); + if (poGeomIn == GetGeometryRef()) + { + return OGRERR_NONE; + } - delete poGeomIn; - return OGRERR_FAILURE; + return SetGeomField(0, std::unique_ptr(poGeomIn)); } /************************************************************************/ @@ -470,7 +471,7 @@ OGRErr OGRFeature::SetGeometryDirectly(OGRGeometry *poGeomIn) /** * \brief Set feature geometry. * - * This function updates the features geometry, and operate exactly as + * This function updates the features geometry, and operates the same as * SetGeometry(), except that this function assumes ownership of the * passed geometry (even in case of failure of that function). * @@ -506,7 +507,7 @@ OGRErr OGR_F_SetGeometryDirectly(OGRFeatureH hFeat, OGRGeometryH hGeom) /** * \brief Set feature geometry. * - * This method updates the features geometry, and operate exactly as + * This method updates the features geometry, and operates the same as * SetGeometryDirectly(), except that this method does not assume ownership * of the passed geometry, but instead makes a copy of it. * @@ -542,7 +543,7 @@ OGRErr OGRFeature::SetGeometry(const OGRGeometry *poGeomIn) /** * \brief Set feature geometry. * - * This function updates the features geometry, and operate exactly as + * This function updates the features geometry, and operates the same as * SetGeometryDirectly(), except that this function does not assume ownership * of the passed geometry, but instead makes a copy of it. * @@ -570,6 +571,37 @@ OGRErr OGR_F_SetGeometry(OGRFeatureH hFeat, OGRGeometryH hGeom) OGRGeometry::FromHandle(hGeom)); } +/************************************************************************/ +/* SetGeometry() */ +/************************************************************************/ + +/** + * \brief Set feature geometry. + * + * This method is the same as the C function OGR_F_SetGeometryDirectly(). + * + * @note This method has only an effect on the in-memory feature object. If + * this object comes from a layer and the modifications must be serialized back + * to the datasource, OGR_L_SetFeature() must be used afterwards. Or if this is + * a new feature, OGR_L_CreateFeature() must be used afterwards. + * + * @param poGeomIn new geometry to apply to feature. Passing NULL value here + * is correct and it will result in deallocation of currently assigned geometry + * without assigning new one. + * + * @return OGRERR_NONE if successful, or OGR_UNSUPPORTED_GEOMETRY_TYPE if + * the geometry type is illegal for the OGRFeatureDefn (checking not yet + * implemented). + * + * @since GDAL 3.11 + */ + +OGRErr OGRFeature::SetGeometry(std::unique_ptr poGeomIn) + +{ + return SetGeomField(0, std::move(poGeomIn)); +} + /************************************************************************/ /* StealGeometry() */ /************************************************************************/ @@ -907,7 +939,7 @@ OGRGeometryH OGR_F_GetGeomFieldRef(OGRFeatureH hFeat, int iField) /** * \brief Set feature geometry of a specified geometry field. * - * This method updates the features geometry, and operate exactly as + * This method updates the features geometry, and operates the same as * SetGeomField(), except that this method assumes ownership of the * passed geometry (even in case of failure of that function). * @@ -926,21 +958,13 @@ OGRGeometryH OGR_F_GetGeomFieldRef(OGRFeatureH hFeat, int iField) */ OGRErr OGRFeature::SetGeomFieldDirectly(int iField, OGRGeometry *poGeomIn) - { - if (iField < 0 || iField >= GetGeomFieldCount()) - { - delete poGeomIn; - return OGRERR_FAILURE; - } - - if (papoGeometries[iField] != poGeomIn) + if (poGeomIn && poGeomIn == GetGeomFieldRef(iField)) { - delete papoGeometries[iField]; - papoGeometries[iField] = poGeomIn; + return OGRERR_NONE; } - return OGRERR_NONE; + return SetGeomField(iField, std::unique_ptr(poGeomIn)); } /************************************************************************/ @@ -950,7 +974,7 @@ OGRErr OGRFeature::SetGeomFieldDirectly(int iField, OGRGeometry *poGeomIn) /** * \brief Set feature geometry of a specified geometry field. * - * This function updates the features geometry, and operate exactly as + * This function updates the features geometry, and operates the same as * SetGeomField(), except that this function assumes ownership of the * passed geometry (even in case of failure of that function). * @@ -985,7 +1009,7 @@ OGRErr OGR_F_SetGeomFieldDirectly(OGRFeatureH hFeat, int iField, /** * \brief Set feature geometry of a specified geometry field. * - * This method updates the features geometry, and operate exactly as + * This method updates the features geometry, and operates the same as * SetGeomFieldDirectly(), except that this method does not assume ownership * of the passed geometry, but instead makes a copy of it. * @@ -1031,7 +1055,7 @@ OGRErr OGRFeature::SetGeomField(int iField, const OGRGeometry *poGeomIn) /** * \brief Set feature geometry of a specified geometry field. * - * This function updates the features geometry, and operate exactly as + * This function updates the features geometry, and operates the same as * SetGeometryDirectly(), except that this function does not assume ownership * of the passed geometry, but instead makes a copy of it. * @@ -1055,6 +1079,45 @@ OGRErr OGR_F_SetGeomField(OGRFeatureH hFeat, int iField, OGRGeometryH hGeom) iField, OGRGeometry::FromHandle(hGeom)); } +/************************************************************************/ +/* SetGeomField() */ +/************************************************************************/ + +/** + * \brief Set feature geometry of a specified geometry field. + * + * This method is the same as the C function OGR_F_SetGeomFieldDirectly(). + * + * @param iField geometry field to set. + * @param poGeomIn new geometry to apply to feature. Passing NULL value here + * is correct and it will result in deallocation of currently assigned geometry + * without assigning new one. + * + * @return OGRERR_NONE if successful, or OGRERR_FAILURE if the index is invalid, + * or OGRERR_UNSUPPORTED_GEOMETRY_TYPE if the geometry type is illegal for the + * OGRFeatureDefn (checking not yet implemented). + * + * @since GDAL 3.11 + */ + +OGRErr OGRFeature::SetGeomField(int iField, + std::unique_ptr poGeomIn) + +{ + if (iField < 0 || iField >= GetGeomFieldCount()) + { + return OGRERR_FAILURE; + } + + if (papoGeometries[iField] != poGeomIn.get()) + { + delete papoGeometries[iField]; + papoGeometries[iField] = poGeomIn.release(); + } + + return OGRERR_NONE; +} + /************************************************************************/ /* Clone() */ /************************************************************************/ @@ -6688,7 +6751,7 @@ const char *OGR_F_GetStyleString(OGRFeatureH hFeat) /** * \brief Set feature style string. * - * This method operate exactly as OGRFeature::SetStyleStringDirectly() except + * This method operates the same as OGRFeature::SetStyleStringDirectly() except * that it does not assume ownership of the passed string, but instead makes a * copy of it. * @@ -6716,7 +6779,7 @@ void OGRFeature::SetStyleString(const char *pszString) /** * \brief Set feature style string. * - * This method operate exactly as OGR_F_SetStyleStringDirectly() except that it + * This method operates the same as OGR_F_SetStyleStringDirectly() except that it * does not assume ownership of the passed string, but instead makes a copy of * it. * @@ -6741,7 +6804,7 @@ void OGR_F_SetStyleString(OGRFeatureH hFeat, const char *pszStyle) /** * \brief Set feature style string. * - * This method operate exactly as OGRFeature::SetStyleString() except that it + * This method operates the same as OGRFeature::SetStyleString() except that it * assumes ownership of the passed string. * * This method is the same as the C function OGR_F_SetStyleStringDirectly(). @@ -6762,7 +6825,7 @@ void OGRFeature::SetStyleStringDirectly(char *pszString) /** * \brief Set feature style string. * - * This method operate exactly as OGR_F_SetStyleString() except that it assumes + * This method operates the same as OGR_F_SetStyleString() except that it assumes * ownership of the passed string. * * This function is the same as the C++ method