From 5e99cf58dd52e7626cdfdff98bf7a70603d0a9bf Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Sun, 24 Nov 2024 16:56:39 +0100 Subject: [PATCH] RFC104: add reference counting to GDALArgDatasetValue::m_poDS --- apps/gdalalg_dispatcher.h | 14 +- apps/gdalalg_pipeline.cpp | 4 +- apps/gdalalg_raster_convert.cpp | 6 +- apps/gdalalg_raster_convert.h | 8 +- apps/gdalalg_raster_info.cpp | 4 +- apps/gdalalg_raster_info.h | 13 +- apps/gdalalg_vector_convert.cpp | 7 +- apps/gdalalg_vector_convert.h | 4 +- apps/gdalalg_vector_filter.cpp | 8 +- apps/gdalalg_vector_info.cpp | 6 +- apps/gdalalg_vector_info.h | 4 +- apps/gdalalg_vector_pipeline.cpp | 12 +- apps/gdalalg_vector_pipeline.h | 2 +- apps/gdalalg_vector_read.cpp | 8 +- apps/gdalalg_vector_reproject.cpp | 6 +- apps/gdalalg_vector_write.cpp | 7 +- autotest/cpp/test_gdal_algorithm.cpp | 110 +++++------- autotest/gcore/algorithm.py | 12 +- autotest/utilities/test_gdalalg_info.py | 2 +- .../utilities/test_gdalalg_raster_convert.py | 2 +- .../utilities/test_gdalalg_raster_info.py | 14 +- gcore/gdalalgorithm.cpp | 169 ++++++++---------- gcore/gdalalgorithm.h | 100 +++++------ swig/include/Algorithm.i | 23 +-- 24 files changed, 236 insertions(+), 309 deletions(-) diff --git a/apps/gdalalg_dispatcher.h b/apps/gdalalg_dispatcher.h index 8dc9d30eba4b..e5f577a26ad6 100644 --- a/apps/gdalalg_dispatcher.h +++ b/apps/gdalalg_dispatcher.h @@ -83,7 +83,7 @@ bool GDALDispatcherAlgorithm:: if (ok) { - auto poDS = m_rasterDispatcher->GetDataset(); + auto poDS = m_rasterDispatcher->GetDatasetRef(); // cppcheck-suppress knownConditionTrueFalse if (poDS && (poDS->GetRasterCount() > 0 || poDS->GetMetadata("SUBDATASETS"))) @@ -112,11 +112,11 @@ bool GDALDispatcherAlgorithm:: return false; } - auto poDSFromRaster = m_rasterDispatcher->GetDataset(); + auto poDSFromRaster = m_rasterDispatcher->GetDatasetRef(); // cppcheck-suppress knownConditionTrueFalse if (poDSFromRaster) { - m_vectorDispatcher->SetDataset(poDSFromRaster, false); + m_vectorDispatcher->SetDataset(poDSFromRaster); } std::vector argsWithoutInput; @@ -180,7 +180,9 @@ bool GDALDispatcherAlgorithm:: return false; } m_rasterDispatcher = std::make_unique(); - m_rasterDispatcher->SetDataset(poDS.release(), true); + auto poDSRaw = poDS.get(); + m_rasterDispatcher->SetDataset(poDS.release()); + poDSRaw->Release(); m_selectedSubAlg = m_rasterDispatcher.get(); std::vector callPath(m_callPath); callPath.push_back("raster"); @@ -192,7 +194,9 @@ bool GDALDispatcherAlgorithm:: else if (poDS->GetLayerCount() != 0) { m_vectorDispatcher = std::make_unique(); - m_vectorDispatcher->SetDataset(poDS.release(), true); + auto poDSRaw = poDS.get(); + m_vectorDispatcher->SetDataset(poDS.release()); + poDSRaw->Release(); m_selectedSubAlg = m_vectorDispatcher.get(); std::vector callPath(m_callPath); callPath.push_back("vector"); diff --git a/apps/gdalalg_pipeline.cpp b/apps/gdalalg_pipeline.cpp index a348bd70e571..528592d5ea3d 100644 --- a/apps/gdalalg_pipeline.cpp +++ b/apps/gdalalg_pipeline.cpp @@ -44,13 +44,13 @@ class GDALDummyRasterPipelineAlgorithm final : public GDALAlgorithm } /* cppcheck-suppress functionStatic */ - GDALDataset *GetDataset() + GDALDataset *GetDatasetRef() { return nullptr; } /* cppcheck-suppress functionStatic */ - void SetDataset(GDALDataset *, bool) + void SetDataset(GDALDataset *) { } diff --git a/apps/gdalalg_raster_convert.cpp b/apps/gdalalg_raster_convert.cpp index fe90b7052516..54892ed33e58 100644 --- a/apps/gdalalg_raster_convert.cpp +++ b/apps/gdalalg_raster_convert.cpp @@ -58,8 +58,8 @@ GDALRasterConvertAlgorithm::GDALRasterConvertAlgorithm( bool GDALRasterConvertAlgorithm::RunImpl(GDALProgressFunc pfnProgress, void *pProgressData) { - CPLAssert(m_inputDataset.GetDataset()); - if (m_outputDataset.GetDataset()) + CPLAssert(m_inputDataset.GetDatasetRef()); + if (m_outputDataset.GetDatasetRef()) { CPLError(CE_Failure, CPLE_NotSupported, "gdal raster convert does not support outputting to an " @@ -94,7 +94,7 @@ bool GDALRasterConvertAlgorithm::RunImpl(GDALProgressFunc pfnProgress, auto poOutDS = std::unique_ptr(GDALDataset::FromHandle( GDALTranslate(m_outputDataset.GetName().c_str(), - GDALDataset::ToHandle(m_inputDataset.GetDataset()), + GDALDataset::ToHandle(m_inputDataset.GetDatasetRef()), psOptions, nullptr))); GDALTranslateOptionsFree(psOptions); if (!poOutDS) diff --git a/apps/gdalalg_raster_convert.h b/apps/gdalalg_raster_convert.h index 61d7d4650d26..7306be5bed12 100644 --- a/apps/gdalalg_raster_convert.h +++ b/apps/gdalalg_raster_convert.h @@ -35,15 +35,15 @@ class GDALRasterConvertAlgorithm final : public GDALAlgorithm explicit GDALRasterConvertAlgorithm(bool openForMixedRasterVector = false); - GDALDataset *GetDataset() + GDALDataset *GetDatasetRef() { - return m_inputDataset.GetDataset(); + return m_inputDataset.GetDatasetRef(); } - void SetDataset(GDALDataset *poDS, bool owned) + void SetDataset(GDALDataset *poDS) { auto arg = GetArg(GDAL_ARG_NAME_INPUT); - arg->Set(poDS, owned); + arg->Set(poDS); arg->SetSkipIfAlreadySet(); } diff --git a/apps/gdalalg_raster_info.cpp b/apps/gdalalg_raster_info.cpp index cda049f19aaf..db38eef37db2 100644 --- a/apps/gdalalg_raster_info.cpp +++ b/apps/gdalalg_raster_info.cpp @@ -93,7 +93,7 @@ GDALRasterInfoAlgorithm::GDALRasterInfoAlgorithm(bool openForMixedRasterVector) bool GDALRasterInfoAlgorithm::RunImpl(GDALProgressFunc, void *) { - CPLAssert(m_dataset.GetDataset()); + CPLAssert(m_dataset.GetDatasetRef()); CPLStringList aosOptions; if (m_format == "json") @@ -128,7 +128,7 @@ bool GDALRasterInfoAlgorithm::RunImpl(GDALProgressFunc, void *) aosOptions.AddString(m_mdd.c_str()); } - GDALDatasetH hDS = GDALDataset::ToHandle(m_dataset.GetDataset()); + GDALDatasetH hDS = GDALDataset::ToHandle(m_dataset.GetDatasetRef()); std::unique_ptr poSubDataset; if (m_subDS > 0) diff --git a/apps/gdalalg_raster_info.h b/apps/gdalalg_raster_info.h index 2ee61d81ecac..5ba16027ce2d 100644 --- a/apps/gdalalg_raster_info.h +++ b/apps/gdalalg_raster_info.h @@ -36,16 +36,19 @@ class GDALRasterInfoAlgorithm final : public GDALAlgorithm explicit GDALRasterInfoAlgorithm(bool openForMixedRasterVector = false); - GDALDataset *GetDataset() + GDALDataset *GetDatasetRef() { - return m_dataset.GetDataset(); + return m_dataset.GetDatasetRef(); } - void SetDataset(GDALDataset *poDS, bool owned) + void SetDataset(GDALDataset *poDS) { auto arg = GetArg(GDAL_ARG_NAME_INPUT); - arg->Set(poDS, owned); - arg->SetSkipIfAlreadySet(); + if (arg) + { + arg->Set(poDS); + arg->SetSkipIfAlreadySet(); + } } private: diff --git a/apps/gdalalg_vector_convert.cpp b/apps/gdalalg_vector_convert.cpp index 70bb502755cf..210098471e90 100644 --- a/apps/gdalalg_vector_convert.cpp +++ b/apps/gdalalg_vector_convert.cpp @@ -63,7 +63,7 @@ GDALVectorConvertAlgorithm::GDALVectorConvertAlgorithm() bool GDALVectorConvertAlgorithm::RunImpl(GDALProgressFunc pfnProgress, void *pProgressData) { - CPLAssert(m_inputDataset.GetDataset()); + CPLAssert(m_inputDataset.GetDatasetRef()); CPLStringList aosOptions; aosOptions.AddString("--invoked-from-gdal-vector-convert"); @@ -116,8 +116,9 @@ bool GDALVectorConvertAlgorithm::RunImpl(GDALProgressFunc pfnProgress, GDALVectorTranslateOptionsSetProgress(psOptions, pfnProgress, pProgressData); - GDALDatasetH hOutDS = GDALDataset::ToHandle(m_outputDataset.GetDataset()); - GDALDatasetH hSrcDS = GDALDataset::ToHandle(m_inputDataset.GetDataset()); + GDALDatasetH hOutDS = + GDALDataset::ToHandle(m_outputDataset.GetDatasetRef()); + GDALDatasetH hSrcDS = GDALDataset::ToHandle(m_inputDataset.GetDatasetRef()); auto poRetDS = GDALDataset::FromHandle( GDALVectorTranslate(m_outputDataset.GetName().c_str(), hOutDS, 1, &hSrcDS, psOptions, nullptr)); diff --git a/apps/gdalalg_vector_convert.h b/apps/gdalalg_vector_convert.h index ded92ae1ee44..f6ce45c3b08d 100644 --- a/apps/gdalalg_vector_convert.h +++ b/apps/gdalalg_vector_convert.h @@ -35,12 +35,12 @@ class GDALVectorConvertAlgorithm final : public GDALAlgorithm GDALVectorConvertAlgorithm(); - void SetDataset(GDALDataset *poDS, bool owned) + void SetDataset(GDALDataset *poDS) { auto arg = GetArg(GDAL_ARG_NAME_INPUT); if (arg) { - arg->Set(poDS, owned); + arg->Set(poDS); arg->SetSkipIfAlreadySet(); } } diff --git a/apps/gdalalg_vector_filter.cpp b/apps/gdalalg_vector_filter.cpp index 18a83d991d43..946493e752d6 100644 --- a/apps/gdalalg_vector_filter.cpp +++ b/apps/gdalalg_vector_filter.cpp @@ -56,9 +56,9 @@ GDALVectorFilterAlgorithm::GDALVectorFilterAlgorithm() bool GDALVectorFilterAlgorithm::RunImpl(GDALProgressFunc, void *) { - CPLAssert(m_inputDataset.GetDataset()); + CPLAssert(m_inputDataset.GetDatasetRef()); CPLAssert(m_outputDataset.GetName().empty()); - CPLAssert(!m_outputDataset.GetDataset()); + CPLAssert(!m_outputDataset.GetDatasetRef()); bool ret = true; if (m_bbox.size() == 4) @@ -67,7 +67,7 @@ bool GDALVectorFilterAlgorithm::RunImpl(GDALProgressFunc, void *) const double ymin = m_bbox[1]; const double xmax = m_bbox[2]; const double ymax = m_bbox[3]; - auto poSrcDS = m_inputDataset.GetDataset(); + auto poSrcDS = m_inputDataset.GetDatasetRef(); const int nLayerCount = poSrcDS->GetLayerCount(); for (int i = 0; i < nLayerCount; ++i) { @@ -78,7 +78,7 @@ bool GDALVectorFilterAlgorithm::RunImpl(GDALProgressFunc, void *) } } - m_outputDataset.Set(m_inputDataset.GetDataset(), false); + m_outputDataset.Set(m_inputDataset.GetDatasetRef()); return true; } diff --git a/apps/gdalalg_vector_info.cpp b/apps/gdalalg_vector_info.cpp index 00064093ca50..cabb1919fd18 100644 --- a/apps/gdalalg_vector_info.cpp +++ b/apps/gdalalg_vector_info.cpp @@ -62,7 +62,7 @@ GDALVectorInfoAlgorithm::GDALVectorInfoAlgorithm() bool GDALVectorInfoAlgorithm::RunImpl(GDALProgressFunc, void *) { - CPLAssert(m_dataset.GetDataset()); + CPLAssert(m_dataset.GetDatasetRef()); CPLStringList aosOptions; if (m_format == "json") @@ -106,8 +106,8 @@ bool GDALVectorInfoAlgorithm::RunImpl(GDALProgressFunc, void *) GDALVectorInfoOptions *psInfo = GDALVectorInfoOptionsNew(aosOptions.List(), nullptr); - char *ret = - GDALVectorInfo(GDALDataset::ToHandle(m_dataset.GetDataset()), psInfo); + char *ret = GDALVectorInfo(GDALDataset::ToHandle(m_dataset.GetDatasetRef()), + psInfo); GDALVectorInfoOptionsFree(psInfo); if (!ret) return false; diff --git a/apps/gdalalg_vector_info.h b/apps/gdalalg_vector_info.h index a10af9a9d5bb..1b520765e12b 100644 --- a/apps/gdalalg_vector_info.h +++ b/apps/gdalalg_vector_info.h @@ -36,12 +36,12 @@ class GDALVectorInfoAlgorithm final : public GDALAlgorithm GDALVectorInfoAlgorithm(); - void SetDataset(GDALDataset *poDS, bool owned) + void SetDataset(GDALDataset *poDS) { auto arg = GetArg(GDAL_ARG_NAME_INPUT); if (arg) { - arg->Set(poDS, owned); + arg->Set(poDS); arg->SetSkipIfAlreadySet(); } } diff --git a/apps/gdalalg_vector_pipeline.cpp b/apps/gdalalg_vector_pipeline.cpp index 8e88c884877f..2c54b0091431 100644 --- a/apps/gdalalg_vector_pipeline.cpp +++ b/apps/gdalalg_vector_pipeline.cpp @@ -397,7 +397,7 @@ bool GDALVectorPipelineAlgorithm::RunImpl(GDALProgressFunc pfnProgress, auto &step = m_steps[i]; if (i > 0) { - if (step->m_inputDataset.GetDataset()) + if (step->m_inputDataset.GetDatasetRef()) { // Shouldn't happen ReportError(CE_Failure, CPLE_AppDefined, @@ -405,9 +405,9 @@ bool GDALVectorPipelineAlgorithm::RunImpl(GDALProgressFunc pfnProgress, static_cast(i), step->GetName().c_str()); return false; } - step->m_inputDataset.Set(poCurDS, false); + step->m_inputDataset.Set(poCurDS); } - if (i + 1 < m_steps.size() && step->m_outputDataset.GetDataset()) + if (i + 1 < m_steps.size() && step->m_outputDataset.GetDatasetRef()) { // Shouldn't happen ReportError(CE_Failure, CPLE_AppDefined, @@ -420,7 +420,7 @@ bool GDALVectorPipelineAlgorithm::RunImpl(GDALProgressFunc pfnProgress, { return false; } - poCurDS = step->m_outputDataset.GetDataset(); + poCurDS = step->m_outputDataset.GetDatasetRef(); if (!poCurDS) { ReportError(CE_Failure, CPLE_AppDefined, @@ -430,9 +430,9 @@ bool GDALVectorPipelineAlgorithm::RunImpl(GDALProgressFunc pfnProgress, } } - if (!m_outputDataset.GetDataset()) + if (!m_outputDataset.GetDatasetRef()) { - m_outputDataset.Set(poCurDS, false); + m_outputDataset.Set(poCurDS); } return true; diff --git a/apps/gdalalg_vector_pipeline.h b/apps/gdalalg_vector_pipeline.h index 499a74820250..0f630148be48 100644 --- a/apps/gdalalg_vector_pipeline.h +++ b/apps/gdalalg_vector_pipeline.h @@ -95,7 +95,7 @@ class GDALVectorPipelineAlgorithm final : public GDALVectorPipelineStepAlgorithm std::string GetUsageAsJSON() const override; /* cppcheck-suppress functionStatic */ - void SetDataset(GDALDataset *, bool) + void SetDataset(GDALDataset *) { } diff --git a/apps/gdalalg_vector_read.cpp b/apps/gdalalg_vector_read.cpp index 7ba97287c106..b955d009de6a 100644 --- a/apps/gdalalg_vector_read.cpp +++ b/apps/gdalalg_vector_read.cpp @@ -67,17 +67,17 @@ class GDALVectorReadAlgorithmDataset final : public GDALDataset bool GDALVectorReadAlgorithm::RunImpl(GDALProgressFunc, void *) { - CPLAssert(m_inputDataset.GetDataset()); + CPLAssert(m_inputDataset.GetDatasetRef()); CPLAssert(m_outputDataset.GetName().empty()); - CPLAssert(!m_outputDataset.GetDataset()); + CPLAssert(!m_outputDataset.GetDatasetRef()); if (m_inputLayerNames.empty()) { - m_outputDataset.Set(m_inputDataset.GetDataset(), false); + m_outputDataset.Set(m_inputDataset.GetDatasetRef()); } else { - auto poSrcDS = m_inputDataset.GetDataset(); + auto poSrcDS = m_inputDataset.GetDatasetRef(); auto poOutDS = std::make_unique(); poOutDS->SetDescription(poSrcDS->GetDescription()); for (const auto &srcLayerName : m_inputLayerNames) diff --git a/apps/gdalalg_vector_reproject.cpp b/apps/gdalalg_vector_reproject.cpp index b76b21809665..c5247e66cc38 100644 --- a/apps/gdalalg_vector_reproject.cpp +++ b/apps/gdalalg_vector_reproject.cpp @@ -73,9 +73,9 @@ class GDALVectorReprojectAlgorithmDataset final : public GDALDataset bool GDALVectorReprojectAlgorithm::RunImpl(GDALProgressFunc, void *) { - CPLAssert(m_inputDataset.GetDataset()); + CPLAssert(m_inputDataset.GetDatasetRef()); CPLAssert(m_outputDataset.GetName().empty()); - CPLAssert(!m_outputDataset.GetDataset()); + CPLAssert(!m_outputDataset.GetDatasetRef()); std::unique_ptr poSrcCRS; if (!m_srsCrs.empty()) @@ -99,7 +99,7 @@ bool GDALVectorReprojectAlgorithm::RunImpl(GDALProgressFunc, void *) } oDstCRS.SetAxisMappingStrategy(OAMS_TRADITIONAL_GIS_ORDER); - auto poSrcDS = m_inputDataset.GetDataset(); + auto poSrcDS = m_inputDataset.GetDatasetRef(); auto reprojectedDataset = std::make_unique(); diff --git a/apps/gdalalg_vector_write.cpp b/apps/gdalalg_vector_write.cpp index ea92696b5abf..5f335b89d942 100644 --- a/apps/gdalalg_vector_write.cpp +++ b/apps/gdalalg_vector_write.cpp @@ -40,7 +40,7 @@ GDALVectorWriteAlgorithm::GDALVectorWriteAlgorithm() bool GDALVectorWriteAlgorithm::RunImpl(GDALProgressFunc pfnProgress, void *pProgressData) { - CPLAssert(m_inputDataset.GetDataset()); + CPLAssert(m_inputDataset.GetDatasetRef()); CPLStringList aosOptions; aosOptions.AddString("--invoked-from-gdal-vector-convert"); @@ -86,8 +86,9 @@ bool GDALVectorWriteAlgorithm::RunImpl(GDALProgressFunc pfnProgress, GDALVectorTranslateOptionsSetProgress(psOptions, pfnProgress, pProgressData); - GDALDatasetH hOutDS = GDALDataset::ToHandle(m_outputDataset.GetDataset()); - GDALDatasetH hSrcDS = GDALDataset::ToHandle(m_inputDataset.GetDataset()); + GDALDatasetH hOutDS = + GDALDataset::ToHandle(m_outputDataset.GetDatasetRef()); + GDALDatasetH hSrcDS = GDALDataset::ToHandle(m_inputDataset.GetDatasetRef()); auto poRetDS = GDALDataset::FromHandle( GDALVectorTranslate(m_outputDataset.GetName().c_str(), hOutDS, 1, &hSrcDS, psOptions, nullptr)); diff --git a/autotest/cpp/test_gdal_algorithm.cpp b/autotest/cpp/test_gdal_algorithm.cpp index de918aec7679..2bbcf0173e14 100644 --- a/autotest/cpp/test_gdal_algorithm.cpp +++ b/autotest/cpp/test_gdal_algorithm.cpp @@ -249,7 +249,7 @@ TEST_F(test_gdal_algorithm, GDALAlgorithmArg_Set) { CPLErrorStateBackuper oBackuper(CPLQuietErrorHandler); CPLErrorReset(); - arg.Set(static_cast(nullptr), false); + arg.Set(static_cast(nullptr)); EXPECT_EQ(CPLGetLastErrorType(), CE_Failure); EXPECT_STREQ(arg.Get().c_str(), "foo"); } @@ -276,25 +276,19 @@ TEST_F(test_gdal_algorithm, GDALAlgorithmArg_Set) GDALArgDatasetValue val; auto arg = GDALAlgorithmArg( GDALAlgorithmArgDecl("", 0, "", GAAT_DATASET), &val); - - arg.Set(poMEMDS.get(), false); - EXPECT_EQ(val.GetDataset(), poMEMDS.get()); - EXPECT_FALSE(val.IsOwned()); auto poMEMDSRaw = poMEMDS.get(); - arg.Set(poMEMDS.release(), true); - EXPECT_EQ(val.GetDataset(), poMEMDSRaw); - EXPECT_TRUE(val.IsOwned()); + arg.Set(poMEMDS.release()); + EXPECT_EQ(val.GetDatasetRef(), poMEMDSRaw); - bool isOwned = false; - poMEMDS.reset(val.BorrowDataset(isOwned)); - EXPECT_TRUE(isOwned); + poMEMDS.reset(val.BorrowDataset()); EXPECT_EQ(poMEMDS.get(), poMEMDSRaw); - EXPECT_EQ(val.GetDataset(), nullptr); + EXPECT_EQ(val.GetDatasetRef(), nullptr); EXPECT_TRUE(arg.Set(std::move(poMEMDS))); - EXPECT_EQ(val.GetDataset(), poMEMDSRaw); - EXPECT_TRUE(val.IsOwned()); + EXPECT_EQ(val.GetDatasetRef(), poMEMDSRaw); + + poMEMDSRaw->ReleaseRef(); arg.SetDatasetName("foo"); EXPECT_STREQ(val.GetName().c_str(), "foo"); @@ -315,13 +309,7 @@ TEST_F(test_gdal_algorithm, GDALAlgorithmArg_Set) { CPLErrorStateBackuper oBackuper(CPLQuietErrorHandler); CPLErrorReset(); - arg.Set(static_cast(nullptr), false); - EXPECT_EQ(CPLGetLastErrorType(), CE_Failure); - } - { - CPLErrorStateBackuper oBackuper(CPLQuietErrorHandler); - CPLErrorReset(); - arg.Set(std::unique_ptr(nullptr)); + arg.Set(static_cast(nullptr)); EXPECT_EQ(CPLGetLastErrorType(), CE_Failure); } } @@ -628,27 +616,18 @@ TEST_F(test_gdal_algorithm, GDALArgDatasetValue) auto poDS = std::unique_ptr( GetGDALDriverManager()->GetDriverByName("MEM")->Create( "", 1, 1, 1, GDT_Byte, nullptr)); - GDALArgDatasetValue value(poDS.release(), true); - EXPECT_NE(value.GetDataset(), nullptr); - EXPECT_TRUE(value.IsOwned()); - } - { - auto poDS = std::unique_ptr( - GetGDALDriverManager()->GetDriverByName("MEM")->Create( - "", 1, 1, 1, GDT_Byte, nullptr)); - GDALArgDatasetValue value(poDS.get(), false); - EXPECT_EQ(value.GetDataset(), poDS.get()); - EXPECT_FALSE(value.IsOwned()); - EXPECT_STREQ(value.GetName().c_str(), poDS->GetDescription()); + auto poDSRaw = poDS.get(); + GDALArgDatasetValue value(poDS.release()); + EXPECT_EQ(value.GetDatasetRef(), poDSRaw); + EXPECT_STREQ(value.GetName().c_str(), poDSRaw->GetDescription()); GDALArgDatasetValue value2; value2 = std::move(value); - EXPECT_STREQ(value2.GetName().c_str(), poDS->GetDescription()); + EXPECT_STREQ(value2.GetName().c_str(), poDSRaw->GetDescription()); + + poDSRaw->ReleaseRef(); } { - auto poDS = std::unique_ptr( - GetGDALDriverManager()->GetDriverByName("MEM")->Create( - "", 1, 1, 1, GDT_Byte, nullptr)); GDALArgDatasetValue value("foo"); EXPECT_STREQ(value.GetName().c_str(), "foo"); @@ -656,15 +635,6 @@ TEST_F(test_gdal_algorithm, GDALArgDatasetValue) value2 = std::move(value); EXPECT_STREQ(value2.GetName().c_str(), "foo"); } - { - auto poDS = std::unique_ptr( - GetGDALDriverManager()->GetDriverByName("MEM")->Create( - "", 1, 1, 1, GDT_Byte, nullptr)); - auto poDSRaw = poDS.get(); - GDALArgDatasetValue value(std::move(poDS)); - EXPECT_EQ(value.GetDataset(), poDSRaw); - EXPECT_TRUE(value.IsOwned()); - } } TEST_F(test_gdal_algorithm, bool_flag) @@ -943,7 +913,7 @@ TEST_F(test_gdal_algorithm, dataset) MyAlgorithm alg; EXPECT_TRUE(alg.ParseCommandLineArguments( {"--val=" GCORE_DATA_DIR "byte.tif"})); - EXPECT_NE(alg.m_val.GetDataset(), nullptr); + EXPECT_NE(alg.m_val.GetDatasetRef(), nullptr); } { @@ -952,8 +922,9 @@ TEST_F(test_gdal_algorithm, dataset) GetGDALDriverManager()->GetDriverByName("MEM")->Create( "", 1, 1, 1, GDT_Byte, nullptr)); auto poDSRaw = poDS.get(); - alg.GetArg("val")->Set(poDS.release(), true); - EXPECT_EQ(alg.m_val.GetDataset(), poDSRaw); + alg.GetArg("val")->Set(poDS.release()); + EXPECT_EQ(alg.m_val.GetDatasetRef(), poDSRaw); + poDSRaw->ReleaseRef(); } { @@ -1019,8 +990,8 @@ TEST_F(test_gdal_algorithm, input_update) EXPECT_TRUE(!alg.GetUsageAsJSON().empty()); EXPECT_TRUE( alg.ParseCommandLineArguments({"--update", osTmpFilename})); - ASSERT_NE(alg.m_input.GetDataset(), nullptr); - EXPECT_EQ(alg.m_input.GetDataset()->GetAccess(), GA_Update); + ASSERT_NE(alg.m_input.GetDatasetRef(), nullptr); + EXPECT_EQ(alg.m_input.GetDatasetRef()->GetAccess(), GA_Update); alg.Finalize(); @@ -1065,10 +1036,11 @@ TEST_F(test_gdal_algorithm, same_input_output_dataset_sqlite) MyAlgorithm alg; EXPECT_TRUE(alg.ParseCommandLineArguments( {"--update", osTmpFilename, osTmpFilename})); - ASSERT_NE(alg.m_input.GetDataset(), nullptr); - EXPECT_NE(alg.m_output.GetDataset(), nullptr); - EXPECT_EQ(alg.m_input.GetDataset(), alg.m_output.GetDataset()); - EXPECT_EQ(alg.m_input.GetDataset()->GetAccess(), GA_Update); + ASSERT_NE(alg.m_input.GetDatasetRef(), nullptr); + EXPECT_NE(alg.m_output.GetDatasetRef(), nullptr); + EXPECT_EQ(alg.m_input.GetDatasetRef(), + alg.m_output.GetDatasetRef()); + EXPECT_EQ(alg.m_input.GetDatasetRef()->GetAccess(), GA_Update); alg.Finalize(); @@ -1381,7 +1353,7 @@ TEST_F(test_gdal_algorithm, vector_dataset) EXPECT_TRUE(alg.ParseCommandLineArguments( {"--val=" GCORE_DATA_DIR "byte.tif"})); ASSERT_EQ(alg.m_val.size(), 1U); - EXPECT_NE(alg.m_val[0].GetDataset(), nullptr); + EXPECT_NE(alg.m_val[0].GetDatasetRef(), nullptr); } { @@ -1391,7 +1363,7 @@ TEST_F(test_gdal_algorithm, vector_dataset) EXPECT_FALSE(alg.ParseCommandLineArguments({"--val=non_existing.tif"})); EXPECT_EQ(CPLGetLastErrorType(), CE_Failure); ASSERT_EQ(alg.m_val.size(), 1U); - EXPECT_EQ(alg.m_val[0].GetDataset(), nullptr); + EXPECT_EQ(alg.m_val[0].GetDatasetRef(), nullptr); } { @@ -1472,8 +1444,8 @@ TEST_F(test_gdal_algorithm, vector_input) {"--update", "--oo=LIST_ALL_TABLES=YES", "--if=GPKG", osTmpFilename})); ASSERT_EQ(alg.m_input.size(), 1U); - ASSERT_NE(alg.m_input[0].GetDataset(), nullptr); - EXPECT_EQ(alg.m_input[0].GetDataset()->GetAccess(), GA_Update); + ASSERT_NE(alg.m_input[0].GetDatasetRef(), nullptr); + EXPECT_EQ(alg.m_input[0].GetDatasetRef()->GetAccess(), GA_Update); alg.Finalize(); @@ -2961,8 +2933,7 @@ TEST_F(test_gdal_algorithm, algorithm_c_api) auto poDS = std::unique_ptr( GetGDALDriverManager()->GetDriverByName("MEM")->Create( "", 1, 1, 1, GDT_Byte, nullptr)); - GDALArgDatasetValueSetDatasetWithoutOwnership(hVal, poDS.get()); - GDALArgDatasetValueSetDatasetWithOwnership(hVal, poDS.release()); + GDALArgDatasetValueSetDataset(hVal, poDS.release()); } GDALAlgorithmArgSetAsDatasetValue(hArg, hVal); @@ -2970,27 +2941,32 @@ TEST_F(test_gdal_algorithm, algorithm_c_api) hVal = GDALAlgorithmArgGetAsDatasetValue(hArg); ASSERT_NE(hVal, nullptr); - EXPECT_NE(GDALArgDatasetValueGetDataset(hVal), nullptr); + auto hDS = GDALArgDatasetValueGetDatasetRef(hVal); + EXPECT_NE(hDS, nullptr); + { + auto hDS2 = GDALArgDatasetValueGetDatasetIncreaseRefCount(hVal); + EXPECT_EQ(hDS2, hDS); + GDALReleaseDataset(hDS2); + } GDALArgDatasetValueRelease(hVal); - GDALAlgorithmArgSetDatasetWithoutOwnership(hArg, nullptr); + GDALAlgorithmArgSetDataset(hArg, nullptr); hVal = GDALAlgorithmArgGetAsDatasetValue(hArg); ASSERT_NE(hVal, nullptr); - EXPECT_EQ(GDALArgDatasetValueGetDataset(hVal), nullptr); + EXPECT_EQ(GDALArgDatasetValueGetDatasetRef(hVal), nullptr); GDALArgDatasetValueRelease(hVal); { auto poDS = std::unique_ptr( GetGDALDriverManager()->GetDriverByName("MEM")->Create( "", 1, 1, 1, GDT_Byte, nullptr)); - GDALAlgorithmArgSetDatasetWithoutOwnership(hArg, poDS.get()); - GDALAlgorithmArgSetDatasetWithOwnership(hArg, poDS.release()); + GDALAlgorithmArgSetDataset(hArg, poDS.release()); } hVal = GDALAlgorithmArgGetAsDatasetValue(hArg); ASSERT_NE(hVal, nullptr); - EXPECT_NE(GDALArgDatasetValueGetDataset(hVal), nullptr); + EXPECT_NE(GDALArgDatasetValueGetDatasetRef(hVal), nullptr); GDALArgDatasetValueRelease(hVal); GDALAlgorithmArgRelease(hArg); diff --git a/autotest/gcore/algorithm.py b/autotest/gcore/algorithm.py index d62b7d4be276..aebc0f634b34 100755 --- a/autotest/gcore/algorithm.py +++ b/autotest/gcore/algorithm.py @@ -163,22 +163,14 @@ def test_algorithm_dataset_value(tmp_path): in_ds = input_arg_value.GetDataset() assert in_ds is not None - assert input_arg_value.IsDatasetOwned() out_ds = output_arg_value.GetDataset() assert out_ds is not None - assert output_arg_value.IsDatasetOwned() assert out_ds.GetRasterBand(1).Checksum() == 4672 - output_arg_value.SetDatasetWithoutOwnership(None) - output_arg_value.SetDatasetWithOwnership(None) + output_arg_value.SetDataset(None) with pytest.raises( Exception, match="Dataset object 'output' is created by algorithm and cannot be set as an input", ): - output_arg.SetDatasetWithoutOwnership(None) - with pytest.raises( - Exception, - match="Dataset object 'output' is created by algorithm and cannot be set as an input", - ): - output_arg.SetDatasetWithOwnership(None) + output_arg.SetDataset(None) diff --git a/autotest/utilities/test_gdalalg_info.py b/autotest/utilities/test_gdalalg_info.py index aca124640240..94d493a67e7d 100755 --- a/autotest/utilities/test_gdalalg_info.py +++ b/autotest/utilities/test_gdalalg_info.py @@ -76,7 +76,7 @@ def test_gdalalg_info_invalid_arg(): def test_gdalalg_info_run_cannot_be_run(): info = get_info_alg() ds = gdal.GetDriverByName("MEM").Create("", 1, 1) - info.GetArg("input").SetDatasetWithoutOwnership(ds) + info.GetArg("input").SetDataset(ds) with pytest.raises(Exception, match="method should not be called directly"): info.Run() diff --git a/autotest/utilities/test_gdalalg_raster_convert.py b/autotest/utilities/test_gdalalg_raster_convert.py index 6a41db84073a..a774972f1645 100755 --- a/autotest/utilities/test_gdalalg_raster_convert.py +++ b/autotest/utilities/test_gdalalg_raster_convert.py @@ -83,7 +83,7 @@ def test_gdalalg_raster_convert_append(tmp_vsimem): def test_gdalalg_raster_convert_error_output_already_set(): convert = get_convert_alg() ds = gdal.GetDriverByName("MEM").Create("", 1, 1) - convert.GetArg("output").Get().SetDatasetWithoutOwnership(ds) + convert.GetArg("output").Get().SetDataset(ds) assert convert.ParseCommandLineArguments(["data/utmsmall.tif"]) with pytest.raises( Exception, diff --git a/autotest/utilities/test_gdalalg_raster_info.py b/autotest/utilities/test_gdalalg_raster_info.py index 94e65c54de1e..18a8857c9cd7 100755 --- a/autotest/utilities/test_gdalalg_raster_info.py +++ b/autotest/utilities/test_gdalalg_raster_info.py @@ -58,7 +58,7 @@ def test_gdalalg_raster_info_mm_checksum(): def test_gdalalg_raster_info_stats(): info = get_info_alg() ds = gdal.Translate("", "../gcore/data/byte.tif", format="MEM") - info.GetArg("input").SetDatasetWithoutOwnership(ds) + info.GetArg("input").SetDataset(ds) assert info.ParseRunAndFinalize(["--stats"]) output_string = info.GetArg("output-string").Get() j = json.loads(output_string) @@ -68,7 +68,7 @@ def test_gdalalg_raster_info_stats(): def test_gdalalg_raster_info_approx_stats(): info = get_info_alg() ds = gdal.Translate("", "../gcore/data/byte.tif", format="MEM") - info.GetArg("input").SetDatasetWithoutOwnership(ds) + info.GetArg("input").SetDataset(ds) assert info.ParseRunAndFinalize(["--approx-stats"]) output_string = info.GetArg("output-string").Get() j = json.loads(output_string) @@ -78,7 +78,7 @@ def test_gdalalg_raster_info_approx_stats(): def test_gdalalg_raster_info_hist(): info = get_info_alg() ds = gdal.Translate("", "../gcore/data/byte.tif", format="MEM") - info.GetArg("input").SetDatasetWithoutOwnership(ds) + info.GetArg("input").SetDataset(ds) assert info.ParseRunAndFinalize(["--hist"]) output_string = info.GetArg("output-string").Get() j = json.loads(output_string) @@ -88,7 +88,7 @@ def test_gdalalg_raster_info_hist(): def test_gdalalg_raster_info_no_options(): info = get_info_alg() ds = gdal.Translate("", "../gcore/data/byte.tif", format="MEM") - info.GetArg("input").SetDatasetWithoutOwnership(ds) + info.GetArg("input").SetDataset(ds) assert info.ParseRunAndFinalize( ["--no-gcp", "--no-md", "--no-ct", "--no-fl", "--no-nodata", "--no-mask"] ) @@ -98,7 +98,7 @@ def test_gdalalg_raster_info_list_mdd(): info = get_info_alg() ds = gdal.Translate("", "../gcore/data/byte.tif", format="MEM") ds.SetMetadataItem("foo", "bar", "MY_DOMAIN") - info.GetArg("input").SetDatasetWithoutOwnership(ds) + info.GetArg("input").SetDataset(ds) assert info.ParseRunAndFinalize(["--list-mdd"]) output_string = info.GetArg("output-string").Get() j = json.loads(output_string) @@ -109,7 +109,7 @@ def test_gdalalg_raster_info_mdd_all(): info = get_info_alg() ds = gdal.Translate("", "../gcore/data/byte.tif", format="MEM") ds.SetMetadataItem("foo", "bar", "MY_DOMAIN") - info.GetArg("input").SetDatasetWithoutOwnership(ds) + info.GetArg("input").SetDataset(ds) assert info.ParseRunAndFinalize(["--mdd=all"]) output_string = info.GetArg("output-string").Get() j = json.loads(output_string) @@ -149,7 +149,7 @@ def test_gdalalg_raster_info_list_subdataset_error_cannot_open_subdataset(): ds = gdal.GetDriverByName("MEM").Create("", 1, 1) ds.SetMetadataItem("SUBDATASET_1_DESC", "desc", "SUBDATASETS") ds.SetMetadataItem("SUBDATASET_1_NAME", "i_do_not_exist", "SUBDATASETS") - info.GetArg("input").SetDatasetWithoutOwnership(ds) + info.GetArg("input").SetDataset(ds) with pytest.raises( Exception, match="i_do_not_exist", diff --git a/gcore/gdalalgorithm.cpp b/gcore/gdalalgorithm.cpp index 456f5c7da2ec..a3220785a22e 100644 --- a/gcore/gdalalgorithm.cpp +++ b/gcore/gdalalgorithm.cpp @@ -267,7 +267,7 @@ bool GDALAlgorithmArg::Set(double value) return SetInternal(value); } -bool GDALAlgorithmArg::Set(GDALDataset *ds, bool owned) +bool GDALAlgorithmArg::Set(GDALDataset *ds) { if (m_decl.GetType() != GAAT_DATASET) { @@ -288,7 +288,7 @@ bool GDALAlgorithmArg::Set(GDALDataset *ds, bool owned) return false; } m_explicitlySet = true; - val.Set(ds, owned); + val.Set(ds); return RunAllActions(); } @@ -538,20 +538,12 @@ GDALInConstructionAlgorithmArg &GDALInConstructionAlgorithmArg::SetPositional() /* GDALArgDatasetValue::GDALArgDatasetValue() */ /************************************************************************/ -GDALArgDatasetValue::GDALArgDatasetValue(GDALDataset *poDS, bool owned) - : m_poDS(poDS), m_owned(owned), - m_name(m_poDS ? m_poDS->GetDescription() : std::string()), m_nameSet(true) -{ -} - -/************************************************************************/ -/* GDALArgDatasetValue::GDALArgDatasetValue() */ -/************************************************************************/ - -GDALArgDatasetValue::GDALArgDatasetValue(std::unique_ptr poDS) - : m_poDS(poDS.release()), m_owned(true), - m_name(m_poDS ? m_poDS->GetDescription() : std::string()), m_nameSet(true) +GDALArgDatasetValue::GDALArgDatasetValue(GDALDataset *poDS) + : m_poDS(poDS), m_name(m_poDS ? m_poDS->GetDescription() : std::string()), + m_nameSet(true) { + if (m_poDS) + m_poDS->Reference(); } /************************************************************************/ @@ -575,7 +567,6 @@ void GDALArgDatasetValue::Set(std::unique_ptr poDS) { Close(); m_poDS = poDS.release(); - m_owned = true; m_name = m_poDS ? m_poDS->GetDescription() : std::string(); m_nameSet = true; if (m_ownerArg) @@ -586,17 +577,32 @@ void GDALArgDatasetValue::Set(std::unique_ptr poDS) /* GDALArgDatasetValue::Set() */ /************************************************************************/ -void GDALArgDatasetValue::Set(GDALDataset *poDS, bool owned) +void GDALArgDatasetValue::Set(GDALDataset *poDS) { Close(); m_poDS = poDS; - m_owned = owned; + if (m_poDS) + m_poDS->Reference(); m_name = m_poDS ? m_poDS->GetDescription() : std::string(); m_nameSet = true; if (m_ownerArg) m_ownerArg->NotifyValueSet(); } +/************************************************************************/ +/* GDALArgDatasetValue::SetFrom() */ +/************************************************************************/ + +void GDALArgDatasetValue::SetFrom(const GDALArgDatasetValue &other) +{ + Close(); + m_name = other.m_name; + m_nameSet = other.m_nameSet; + m_poDS = other.m_poDS; + if (m_poDS) + m_poDS->Reference(); +} + /************************************************************************/ /* GDALArgDatasetValue::~GDALArgDatasetValue() */ /************************************************************************/ @@ -613,7 +619,7 @@ GDALArgDatasetValue::~GDALArgDatasetValue() bool GDALArgDatasetValue::Close() { bool ret = true; - if (m_owned && m_poDS) + if (m_poDS && m_poDS->Dereference() == 0) { ret = m_poDS->Close() == CE_None; delete m_poDS; @@ -630,30 +636,38 @@ GDALArgDatasetValue &GDALArgDatasetValue::operator=(GDALArgDatasetValue &&other) { Close(); m_poDS = other.m_poDS; - m_owned = other.m_owned; m_name = other.m_name; m_nameSet = other.m_nameSet; m_type = other.m_type; m_inputFlags = other.m_inputFlags; m_outputFlags = other.m_outputFlags; other.m_poDS = nullptr; - other.m_owned = false; other.m_name.clear(); other.m_nameSet = false; return *this; } +/************************************************************************/ +/* GDALArgDatasetValue::GetDataset() */ +/************************************************************************/ + +GDALDataset *GDALArgDatasetValue::GetDatasetIncreaseRefCount() +{ + if (m_poDS) + m_poDS->Reference(); + return m_poDS; +} + /************************************************************************/ /* GDALArgDatasetValue(GDALArgDatasetValue &&other) */ /************************************************************************/ GDALArgDatasetValue::GDALArgDatasetValue(GDALArgDatasetValue &&other) - : m_poDS(other.m_poDS), m_owned(other.m_owned), m_name(other.m_name), - m_nameSet(other.m_nameSet), m_type(other.m_type), - m_inputFlags(other.m_inputFlags), m_outputFlags(other.m_outputFlags) + : m_poDS(other.m_poDS), m_name(other.m_name), m_nameSet(other.m_nameSet), + m_type(other.m_type), m_inputFlags(other.m_inputFlags), + m_outputFlags(other.m_outputFlags) { other.m_poDS = nullptr; - other.m_owned = false; other.m_name.clear(); } @@ -1309,14 +1323,14 @@ bool GDALAlgorithm::ProcessDatasetArg(GDALAlgorithmArg *arg, overwriteArg->GetType() == GAAT_BOOLEAN && overwriteArg->Get()); auto outputArg = algForOutput->GetArg(GDAL_ARG_NAME_OUTPUT); auto &val = arg->Get(); - if (!val.GetDataset() && !val.IsNameSet()) + if (!val.GetDatasetRef() && !val.IsNameSet()) { ReportError(CE_Failure, CPLE_AppDefined, "Argument '%s' has no dataset object or dataset name.", arg->GetName().c_str()); ret = false; } - else if (!val.GetDataset() && + else if (!val.GetDatasetRef() && (!arg->IsOutput() || (arg == outputArg && update && !overwrite))) { int flags = val.GetType(); @@ -1328,7 +1342,7 @@ bool GDALAlgorithm::ProcessDatasetArg(GDALAlgorithmArg *arg, outputArg && outputArg->GetType() == GAAT_DATASET) { auto &outputVal = outputArg->Get(); - if (!outputVal.GetDataset() && + if (!outputVal.GetDatasetRef() && outputVal.GetName() == val.GetName() && (outputVal.GetInputFlags() & GADV_OBJECT) != 0) { @@ -1357,9 +1371,9 @@ bool GDALAlgorithm::ProcessDatasetArg(GDALAlgorithmArg *arg, CPLStringList(ifArg->Get>()); } - auto poDS = std::unique_ptr( + auto poDS = GDALDataset::Open(val.GetName().c_str(), flags, - aosAllowedDrivers.List(), aosOpenOptions.List())); + aosAllowedDrivers.List(), aosOpenOptions.List()); if (poDS) { if (assignToOutputArg) @@ -1376,11 +1390,11 @@ bool GDALAlgorithm::ProcessDatasetArg(GDALAlgorithmArg *arg, EQUAL(poDriver->GetDescription(), "SQLite") || EQUAL(poDriver->GetDescription(), "GPKG"))) { - outputArg->Get().Set(poDS.get(), - false); + outputArg->Get().Set(poDS); } } - val.Set(std::move(poDS)); + val.Set(poDS); + poDS->ReleaseRef(); } else { @@ -1503,7 +1517,7 @@ bool GDALAlgorithm::ValidateArguments() auto &listVal = arg->Get>(); for (auto &val : listVal) { - if (!val.GetDataset() && val.GetName().empty()) + if (!val.GetDatasetRef() && val.GetName().empty()) { ReportError( CE_Failure, CPLE_AppDefined, @@ -1511,7 +1525,7 @@ bool GDALAlgorithm::ValidateArguments() arg->GetName().c_str()); ret = false; } - else if (!val.GetDataset()) + else if (!val.GetDatasetRef()) { int flags = val.GetType() | GDAL_OF_VERBOSE_ERROR; @@ -2085,10 +2099,10 @@ bool GDALAlgorithm::Run(GDALProgressFunc pfnProgress, void *pProgressData) bool GDALAlgorithm::Finalize() { + bool ret = true; if (m_selectedSubAlg) - return m_selectedSubAlg->Finalize(); + ret = m_selectedSubAlg->Finalize(); - bool ret = true; for (auto &arg : m_args) { if (arg->GetType() == GAAT_DATASET) @@ -3661,29 +3675,10 @@ bool GDALAlgorithmArgSetAsDatasetValue(GDALAlgorithmArgH hArg, } /************************************************************************/ -/* GDALAlgorithmArgSetDatasetWithoutOwnership() */ -/************************************************************************/ - -/** Set dataset object, but without transferring ownership to hArg. - * - * @param hArg Handle to an argument. Must NOT be null. - * @param hDS Dataset object. May be null. - * @return true if success. - * @since 3.11 - */ - -bool GDALAlgorithmArgSetDatasetWithoutOwnership(GDALAlgorithmArgH hArg, - GDALDatasetH hDS) -{ - VALIDATE_POINTER1(hArg, __func__, false); - return hArg->ptr->Set(GDALDataset::FromHandle(hDS), false); -} - -/************************************************************************/ -/* GDALAlgorithmArgSetDatasetWithOwnership() */ +/* GDALAlgorithmArgSetDataset() */ /************************************************************************/ -/** Set dataset object, and transfer its ownership to hArg. +/** Set dataset object, increasing its reference counter. * * @param hArg Handle to an argument. Must NOT be null. * @param hDS Dataset object. May be null. @@ -3691,11 +3686,10 @@ bool GDALAlgorithmArgSetDatasetWithoutOwnership(GDALAlgorithmArgH hArg, * @since 3.11 */ -bool GDALAlgorithmArgSetDatasetWithOwnership(GDALAlgorithmArgH hArg, - GDALDatasetH hDS) +bool GDALAlgorithmArgSetDataset(GDALAlgorithmArgH hArg, GDALDatasetH hDS) { VALIDATE_POINTER1(hArg, __func__, false); - return hArg->ptr->Set(GDALDataset::FromHandle(hDS), false); + return hArg->ptr->Set(GDALDataset::FromHandle(hDS)); } /************************************************************************/ @@ -3809,34 +3803,39 @@ const char *GDALArgDatasetValueGetName(GDALArgDatasetValueH hValue) } /************************************************************************/ -/* GDALArgDatasetValueGetDataset() */ +/* GDALArgDatasetValueGetDatasetRef() */ /************************************************************************/ -/** Return the dataset component of the GDALArgDatasetValue +/** Return the dataset component of the GDALArgDatasetValue. + * + * This does not modify the reference counter, hence the lifetime of the + * returned object is not guaranteed to exceed the one of hValue. * * @param hValue Handle to a GDALArgDatasetValue. Must NOT be null. * @since 3.11 */ -GDALDatasetH GDALArgDatasetValueGetDataset(GDALArgDatasetValueH hValue) +GDALDatasetH GDALArgDatasetValueGetDatasetRef(GDALArgDatasetValueH hValue) { VALIDATE_POINTER1(hValue, __func__, nullptr); - return GDALDataset::ToHandle(hValue->ptr->GetDataset()); + return GDALDataset::ToHandle(hValue->ptr->GetDatasetRef()); } /************************************************************************/ -/* GDALArgDatasetValueIsDatasetOwned() */ +/* GDALArgDatasetValueGetDatasetIncreaseRefCount() */ /************************************************************************/ -/** Returns whether the dataset object referenced in this object is owned - * by this object (and thus released by it when it is destroyed). +/** Return the dataset component of the GDALArgDatasetValue, and increase its + * reference count if not null. Once done with the dataset, the caller should + * call GDALReleaseDataset(). * * @param hValue Handle to a GDALArgDatasetValue. Must NOT be null. * @since 3.11 */ -bool GDALArgDatasetValueIsDatasetOwned(GDALArgDatasetValueH hValue) +GDALDatasetH +GDALArgDatasetValueGetDatasetIncreaseRefCount(GDALArgDatasetValueH hValue) { - VALIDATE_POINTER1(hValue, __func__, false); - return hValue->ptr->IsOwned(); + VALIDATE_POINTER1(hValue, __func__, nullptr); + return GDALDataset::ToHandle(hValue->ptr->GetDatasetIncreaseRefCount()); } /************************************************************************/ @@ -3925,37 +3924,19 @@ void GDALArgDatasetValueSetName(GDALArgDatasetValueH hValue, } /************************************************************************/ -/* GDALArgDatasetValueSetDatasetWithoutOwnership() */ -/************************************************************************/ - -/** Set dataset object, but without transferring ownership to hValue. - * - * @param hValue Handle to a GDALArgDatasetValue. Must NOT be null. - * @param hDS Dataset object. May be null. - * @since 3.11 - */ - -void GDALArgDatasetValueSetDatasetWithoutOwnership(GDALArgDatasetValueH hValue, - GDALDatasetH hDS) -{ - VALIDATE_POINTER0(hValue, __func__); - hValue->ptr->Set(GDALDataset::FromHandle(hDS), false); -} - -/************************************************************************/ -/* GDALArgDatasetValueSetDatasetWithOwnership() */ +/* GDALArgDatasetValueSetDataset() */ /************************************************************************/ -/** Set dataset object, and transfer its ownership to hValue. +/** Set dataset object, increasing its reference counter. * * @param hValue Handle to a GDALArgDatasetValue. Must NOT be null. * @param hDS Dataset object. May be null. * @since 3.11 */ -void GDALArgDatasetValueSetDatasetWithOwnership(GDALArgDatasetValueH hValue, - GDALDatasetH hDS) +void GDALArgDatasetValueSetDataset(GDALArgDatasetValueH hValue, + GDALDatasetH hDS) { VALIDATE_POINTER0(hValue, __func__); - hValue->ptr->Set(GDALDataset::FromHandle(hDS), true); + hValue->ptr->Set(GDALDataset::FromHandle(hDS)); } diff --git a/gcore/gdalalgorithm.h b/gcore/gdalalgorithm.h index c09390185699..1f6af3f38c68 100644 --- a/gcore/gdalalgorithm.h +++ b/gcore/gdalalgorithm.h @@ -192,11 +192,7 @@ bool CPL_DLL GDALAlgorithmArgSetAsString(GDALAlgorithmArgH, const char *); bool CPL_DLL GDALAlgorithmArgSetAsDatasetValue(GDALAlgorithmArgH hArg, GDALArgDatasetValueH value); -bool CPL_DLL GDALAlgorithmArgSetDatasetWithoutOwnership(GDALAlgorithmArgH hArg, - GDALDatasetH); - -bool CPL_DLL GDALAlgorithmArgSetDatasetWithOwnership(GDALAlgorithmArgH hArg, - GDALDatasetH); +bool CPL_DLL GDALAlgorithmArgSetDataset(GDALAlgorithmArgH hArg, GDALDatasetH); bool CPL_DLL GDALAlgorithmArgSetAsInteger(GDALAlgorithmArgH, int); @@ -220,9 +216,10 @@ void CPL_DLL GDALArgDatasetValueRelease(GDALArgDatasetValueH); const char CPL_DLL *GDALArgDatasetValueGetName(GDALArgDatasetValueH); -GDALDatasetH CPL_DLL GDALArgDatasetValueGetDataset(GDALArgDatasetValueH); +GDALDatasetH CPL_DLL GDALArgDatasetValueGetDatasetRef(GDALArgDatasetValueH); -bool CPL_DLL GDALArgDatasetValueIsDatasetOwned(GDALArgDatasetValueH hValue); +GDALDatasetH + CPL_DLL GDALArgDatasetValueGetDatasetIncreaseRefCount(GDALArgDatasetValueH); /** Bit indicating that the name component of GDALArgDatasetValue is accepted. */ #define GADV_NAME (1 << 0) @@ -243,11 +240,7 @@ int CPL_DLL GDALArgDatasetValueGetOutputFlags(GDALArgDatasetValueH); void CPL_DLL GDALArgDatasetValueSetName(GDALArgDatasetValueH, const char *); -void CPL_DLL GDALArgDatasetValueSetDatasetWithoutOwnership(GDALArgDatasetValueH, - GDALDatasetH); - -void CPL_DLL GDALArgDatasetValueSetDatasetWithOwnership(GDALArgDatasetValueH, - GDALDatasetH); +void CPL_DLL GDALArgDatasetValueSetDataset(GDALArgDatasetValueH, GDALDatasetH); CPL_C_END @@ -325,61 +318,55 @@ class CPL_DLL GDALArgDatasetValue final { } - /** Constructor by dataset instance. */ - explicit GDALArgDatasetValue(GDALDataset *poDS, bool owned); - - /** Constructor by dataset instance. */ - explicit GDALArgDatasetValue(std::unique_ptr poDS); + /** Constructor by dataset instance, increasing its reference counter */ + explicit GDALArgDatasetValue(GDALDataset *poDS); /** Move constructor */ GDALArgDatasetValue(GDALArgDatasetValue &&other); - /** Destructor. Free m_poDS if m_owned is true. */ + /** Destructor. Decrease m_poDS reference count, and destroy it if no + * longer referenced. */ ~GDALArgDatasetValue(); - /** Close the dataset if it is owned and return an error if an error - * occurred during dataset closing */ + /** Dereference the dataset object and close it if no longer referenced. + * Return an error if an error occurred during dataset closing. */ bool Close(); /** Move-assignment operator */ GDALArgDatasetValue &operator=(GDALArgDatasetValue &&other); - /** Get GDALDataset* instance (may be null) */ - GDALDataset *GetDataset() - { - return m_poDS; - } + /** Get the GDALDataset* instance (may be null), and increase its reference + * count if not null. Once done with the dataset, the caller should call + * GDALDataset::Release(). + */ + GDALDataset *GetDatasetIncreaseRefCount(); - /** Get GDALDataset* instance (may be null) */ - const GDALDataset *GetDataset() const + /** Get a GDALDataset* instance (may be null). This does not modify the + * reference counter, hence the lifetime of the returned object is not + * guaranteed to exceed the one of this instance. + */ + GDALDataset *GetDatasetRef() { return m_poDS; } - /** Returns whether the dataset object referenced in this object is owned - * by this object (and thus released by it when it is destroyed). + /** Borrow the GDALDataset* instance (may be null), leaving its reference + * counter unchanged. */ - bool IsOwned() const + GDALDataset *BorrowDataset() { - return m_owned; - } - - /** Borrow the GDALDataset* instance (may be null), with its ownership - * status.*/ - GDALDataset *BorrowDataset(bool &owned) - { - owned = m_owned; - m_owned = false; GDALDataset *ret = m_poDS; m_poDS = nullptr; return ret; } - /** Borrow the GDALDataset* instance from another GDALArgDatasetValue */ + /** Borrow the GDALDataset* instance from another GDALArgDatasetValue, + * leaving its reference counter unchange. + */ void BorrowDatasetFrom(GDALArgDatasetValue &other) { Close(); - m_poDS = other.BorrowDataset(m_owned); + m_poDS = other.BorrowDataset(); m_name = other.m_name; } @@ -427,21 +414,17 @@ class CPL_DLL GDALArgDatasetValue final /** Set dataset name */ void Set(const std::string &name); - /** Transfer dataset to this instance. */ + /** Transfer dataset to this instance (does not affect is reference + * counter). */ void Set(std::unique_ptr poDS); - /** Set dataset object. */ - void Set(GDALDataset *poDS, bool owned); + /** Set dataset object, increasing its reference counter. */ + void Set(GDALDataset *poDS); - /** Set from other value, referencing other.m_poDS. */ - void SetFrom(const GDALArgDatasetValue &other) - { - Close(); - m_name = other.m_name; - m_nameSet = other.m_nameSet; - m_poDS = other.m_poDS; - m_owned = false; - } + /** Set from other value, increasing the reference counter of the + * GDALDataset object. + */ + void SetFrom(const GDALArgDatasetValue &other); /** Set which components among name and dataset are accepted as * input, when this argument serves as an input. @@ -493,13 +476,9 @@ class CPL_DLL GDALArgDatasetValue final /** The owner argument (may be nullptr for freestanding objects) */ GDALAlgorithmArg *m_ownerArg = nullptr; - /** Dataset object. Owned by this instance if m_owned is true. */ + /** Dataset object. */ GDALDataset *m_poDS = nullptr; - /** Whether m_poDS should be destroyed at destruction of this - * GDALArgDatasetValue instance. */ - bool m_owned = false; - /** Dataset name */ std::string m_name{}; @@ -1235,12 +1214,13 @@ class CPL_DLL GDALAlgorithmArg /* non-final */ /** Set the value for a GAAT_REAL argument */ bool Set(double value); - /** Set the value for a GAAT_DATASET argument. + /** Set the value for a GAAT_DATASET argument, increasing ds' reference + * counter if ds is not null. * It cannot be called several times for a given argument. * Validation checks and other actions are run. * Return true if success. */ - bool Set(GDALDataset *ds, bool owned); + bool Set(GDALDataset *ds); /** Set the value for a GAAT_DATASET argument. * It cannot be called several times for a given argument. diff --git a/swig/include/Algorithm.i b/swig/include/Algorithm.i index bb4a38828a30..25df9b898d86 100644 --- a/swig/include/Algorithm.i +++ b/swig/include/Algorithm.i @@ -223,12 +223,8 @@ public: return GDALAlgorithmArgSetAsDoubleList(self, nList, pList); } - void SetDatasetWithoutOwnership(GDALDatasetShadow* ds) { - GDALAlgorithmArgSetDatasetWithoutOwnership(self, ds); - } - - void SetDatasetWithOwnership(GDALDatasetShadow* ds) { - GDALAlgorithmArgSetDatasetWithOwnership(self, ds); + void SetDataset(GDALDatasetShadow* ds) { + GDALAlgorithmArgSetDataset(self, ds); } } }; @@ -385,12 +381,9 @@ public: return GDALArgDatasetValueGetName(self); } +%newobject GetDataset; GDALDatasetShadow* GetDataset() { - return GDALArgDatasetValueGetDataset(self); - } - - bool IsDatasetOwned() { - return GDALArgDatasetValueIsDatasetOwned(self); + return GDALArgDatasetValueGetDatasetIncreaseRefCount(self); } int GetInputFlags() { @@ -405,12 +398,8 @@ public: GDALArgDatasetValueSetName(self, name); } - void SetDatasetWithoutOwnership(GDALDatasetShadow* ds) { - GDALArgDatasetValueSetDatasetWithoutOwnership(self, ds); - } - - void SetDatasetWithOwnership(GDALDatasetShadow* ds) { - GDALArgDatasetValueSetDatasetWithOwnership(self, ds); + void SetDataset(GDALDatasetShadow* ds) { + GDALArgDatasetValueSetDataset(self, ds); } } };