Merge pull request #24 from ecmwf/feature/unique_exresult

Feature/unique exresult
ecmwf · Oct 30, 2024 · 1282899 · 1282899
2 parents 585b74e + 771c5b5
commit 1282899
Show file tree

Hide file tree

Showing 13 changed files with 45 additions and 32 deletions.
diff --git a/src/gribjump/ExtractionData.h b/src/gribjump/ExtractionData.h
@@ -34,6 +34,13 @@ class ExtractionResult  {
     ExtractionResult(std::vector<std::vector<double>> values, std::vector<std::vector<std::bitset<64>>> mask);
     explicit ExtractionResult(eckit::Stream& s);
 
+    // Movable, not copyable
+    ExtractionResult(const ExtractionResult&) = delete;
+    ExtractionResult& operator=(const ExtractionResult&) = delete;
+
+    ExtractionResult(ExtractionResult&&) = default;
+    ExtractionResult& operator=(ExtractionResult&&) = default;
+
     const std::vector<std::vector<double>>& values() const {return values_;}
     const std::vector<std::vector<std::bitset<64>>>& mask() const {return mask_;}
 

diff --git a/src/gribjump/GribJump.cc b/src/gribjump/GribJump.cc
@@ -48,13 +48,13 @@ size_t GribJump::scan(const std::vector<metkit::mars::MarsRequest> requests, boo
 }
 
 
-std::vector<std::vector<ExtractionResult*>> GribJump::extract(const std::vector<ExtractionRequest>& requests, LogContext ctx) {
+std::vector<std::vector<std::unique_ptr<ExtractionResult>>> GribJump::extract(const std::vector<ExtractionRequest>& requests, LogContext ctx) {
 
     if (requests.empty()) {
         throw eckit::UserError("Requests must not be empty", Here());
     }
 
-    std::vector<std::vector<ExtractionResult*>> out = impl_->extract(requests, ctx);
+    std::vector<std::vector<std::unique_ptr<ExtractionResult>>> out = impl_->extract(requests, ctx); // ... why is this still using raw pointers? // why are we not using extraction items?
     return out;
 }
 

diff --git a/src/gribjump/GribJump.h b/src/gribjump/GribJump.h
@@ -46,7 +46,7 @@ class GribJump {
     size_t scan(const std::vector<eckit::PathName>& paths);
     size_t scan(std::vector<metkit::mars::MarsRequest> requests, bool byfiles = false);
 
-    std::vector<std::vector<ExtractionResult*>> extract(const std::vector<ExtractionRequest>& requests, LogContext ctx=LogContext("none"));
+    std::vector<std::vector<std::unique_ptr<ExtractionResult>>> extract(const std::vector<ExtractionRequest>& requests, LogContext ctx=LogContext("none"));
     std::vector<std::unique_ptr<ExtractionItem>> extract(const eckit::PathName& path, const std::vector<eckit::Offset>& offsets, const std::vector<std::vector<Range>>& ranges);
 
     std::map<std::string, std::unordered_set<std::string>> axes(const std::string& request);

diff --git a/src/gribjump/GribJumpBase.h b/src/gribjump/GribJumpBase.h
@@ -47,7 +47,7 @@ class GribJumpBase : public eckit::NonCopyable {
 
     virtual size_t scan(const std::vector<metkit::mars::MarsRequest> requests, bool byfiles) = 0;
 
-    virtual std::vector<std::vector<ExtractionResult*>> extract(std::vector<ExtractionRequest>, LogContext ctx=LogContext("none")) = 0;
+    virtual std::vector<std::vector<std::unique_ptr<ExtractionResult>>> extract(std::vector<ExtractionRequest>, LogContext ctx=LogContext("none")) = 0;
     virtual std::vector<std::unique_ptr<ExtractionItem>> extract(const eckit::PathName& path, const std::vector<eckit::Offset>& offsets, const std::vector<std::vector<Range>>& ranges) = 0;
 
     virtual std::map<std::string, std::unordered_set<std::string>> axes(const std::string& request) = 0;

diff --git a/src/gribjump/LocalGribJump.cc b/src/gribjump/LocalGribJump.cc
@@ -84,24 +84,24 @@ std::vector<std::unique_ptr<ExtractionItem>> LocalGribJump::extract(const eckit:
 }
 
 /// @todo, change API, remove extraction request
-std::vector<std::vector<ExtractionResult*>> LocalGribJump::extract(ExtractionRequests requests, LogContext ctx) {
+std::vector<std::vector<std::unique_ptr<ExtractionResult>>> LocalGribJump::extract(ExtractionRequests requests, LogContext ctx) {
 
     bool flatten = true;
     Engine engine;
     ResultsMap results = engine.extract(requests, flatten);
     engine.raiseErrors();
 
-    std::vector<std::vector<ExtractionResult*>> extractionResults;
+    std::vector<std::vector<std::unique_ptr<ExtractionResult>>> extractionResults;
     for (auto& req : requests) {
         auto it = results.find(req.request());
         ASSERT(it != results.end());
-        std::vector<ExtractionResult*> res;
+        std::vector<std::unique_ptr<ExtractionResult>> res;
         for (auto& item : it->second) {
-            ExtractionResult* r = new ExtractionResult(item->values(), item->mask());
-            res.push_back(r);
+            // std::unique_ptr<ExtractionResult> r(new ExtractionResult(item->values(), item->mask()));
+            res.push_back(std::make_unique<ExtractionResult>(item->values(), item->mask()));
         }
 
-        extractionResults.push_back(res);
+        extractionResults.push_back(std::move(res));
     }
 
     return extractionResults;

diff --git a/src/gribjump/LocalGribJump.h b/src/gribjump/LocalGribJump.h
@@ -39,7 +39,7 @@ class LocalGribJump : public GribJumpBase {
 
     // old API
     std::vector<std::unique_ptr<ExtractionItem>> extract(const eckit::PathName& path, const std::vector<eckit::Offset>& offsets, const std::vector<std::vector<Range>>& ranges) override;
-    std::vector<std::vector<ExtractionResult*>> extract(std::vector<ExtractionRequest>, LogContext ctx=LogContext("none")) override;
+    std::vector<std::vector<std::unique_ptr<ExtractionResult>>> extract(std::vector<ExtractionRequest>, LogContext ctx=LogContext("none")) override;
 
     std::map<std::string, std::unordered_set<std::string>> axes(const std::string& request) override;
 

diff --git a/src/gribjump/gribjump_c.cc b/src/gribjump/gribjump_c.cc
@@ -45,7 +45,7 @@ struct gribjump_handle_t : public GribJump {
 struct gribjump_extraction_result_t: public ExtractionResult {
     using ExtractionResult::ExtractionResult;
 
-    gribjump_extraction_result_t(const ExtractionResult& result): ExtractionResult(result) {}
+    explicit gribjump_extraction_result_t(std::unique_ptr<ExtractionResult> result): ExtractionResult(std::move(*result)) {}
 };
 
 // todo deleter
@@ -154,13 +154,16 @@ int gribjump_delete_result(gribjump_extraction_result_t* result) {
 /// @todo review why this extract_single exists.
 int extract_single(gribjump_handle_t* handle, gribjump_extraction_request_t* request, gribjump_extraction_result_t*** results_array, unsigned long* nfields) {
     ExtractionRequest req = *request;
-    std::vector<ExtractionResult*> results = handle->extract(std::vector<ExtractionRequest>{req})[0];
+    std::vector<std::vector<std::unique_ptr<ExtractionResult>>> resultsv = handle->extract(std::vector<ExtractionRequest>{req});
+    ASSERT(resultsv.size() == 1);
+
+    std::vector<std::unique_ptr<ExtractionResult>> results = std::move(resultsv[0]);
 
     *nfields = results.size();
     *results_array = new gribjump_extraction_result_t*[*nfields];
 
     for (size_t i = 0; i < *nfields; i++) {
-        (*results_array)[i] = new gribjump_extraction_result_t(*results[i]);
+        (*results_array)[i] = new gribjump_extraction_result_t(std::move(results[i])); // not convinced this is safe
     }
 
     return 0;
@@ -175,7 +178,7 @@ int extract(gribjump_handle_t* handle, gribjump_extraction_request_t** requests,
         logctx = LogContext(ctx);
     }
 
-    std::vector<std::vector<ExtractionResult*>> results;
+    std::vector<std::vector<std::unique_ptr<ExtractionResult>>> results;
     try {
         results = handle->extract(reqs, logctx);
     } catch (std::exception& e) {
@@ -196,7 +199,7 @@ int extract(gribjump_handle_t* handle, gribjump_extraction_request_t** requests,
         (*nfields)[i] = results[i].size();
         (*results_array)[i] = new gribjump_extraction_result_t*[(*nfields)[i]];
         for (size_t j = 0; j < (*nfields)[i]; j++) {
-            (*results_array)[i][j] = new gribjump_extraction_result_t(*results[i][j]);
+            (*results_array)[i][j] = new gribjump_extraction_result_t(std::move(results[i][j]));
         }
     }
     return 0;

diff --git a/src/gribjump/remote/RemoteGribJump.cc b/src/gribjump/remote/RemoteGribJump.cc
@@ -78,9 +78,9 @@ size_t RemoteGribJump::scan(const std::vector<metkit::mars::MarsRequest> request
     return count;
 }
 
-std::vector<std::vector<ExtractionResult*>> RemoteGribJump::extract(std::vector<ExtractionRequest> requests, LogContext ctx) {
+std::vector<std::vector<std::unique_ptr<ExtractionResult>>> RemoteGribJump::extract(std::vector<ExtractionRequest> requests, LogContext ctx) {
     eckit::Timer timer("RemoteGribJump::extract()");
-    std::vector<std::vector<ExtractionResult*>> result;
+    std::vector<std::vector<std::unique_ptr<ExtractionResult>>> result;
 
     // connect to server
     eckit::net::TCPClient client;
@@ -104,13 +104,13 @@ std::vector<std::vector<ExtractionResult*>> RemoteGribJump::extract(std::vector<
     bool error = receiveErrors(stream);
 
     for (size_t i = 0; i < nRequests; i++) {
-        std::vector<ExtractionResult*> response;
+        std::vector<std::unique_ptr<ExtractionResult>> response;
         size_t nfields;
         stream >> nfields;
         for (size_t i = 0; i < nfields; i++) {
-            response.push_back(new ExtractionResult(stream));
+            response.push_back(std::make_unique<ExtractionResult>(stream));
         }
-        result.push_back(response);
+        result.push_back(std::move(response));
     }
     timer.report("All data recieved");
     return result;

diff --git a/src/gribjump/remote/RemoteGribJump.h b/src/gribjump/remote/RemoteGribJump.h
@@ -38,7 +38,7 @@ class RemoteGribJump : public GribJumpBase {
 
     size_t scan(const std::vector<metkit::mars::MarsRequest> requests, bool byfiles) override;
 
-    std::vector<std::vector<ExtractionResult*>> extract(std::vector<ExtractionRequest> polyRequest, LogContext ctx) override;
+    std::vector<std::vector<std::unique_ptr<ExtractionResult>>> extract(std::vector<ExtractionRequest> polyRequest, LogContext ctx) override;
     std::vector<std::unique_ptr<ExtractionItem>> extract(const eckit::PathName& path, const std::vector<eckit::Offset>& offsets, const std::vector<std::vector<Range>>& ranges) override;
     void extract(filemap_t& filemap);
 

diff --git a/src/tools/gribjump-extract.cc b/src/tools/gribjump-extract.cc
@@ -94,7 +94,7 @@ void GribJumpExtract::execute(const eckit::option::CmdArgs &args) {
 
     // Extract values
     GribJump gj;
-    std::vector<std::vector<ExtractionResult*>> output = gj.extract(polyRequest);
+    std::vector<std::vector<std::unique_ptr<ExtractionResult>>> output = gj.extract(polyRequest);
 
     // Print extracted values
     if (!printout) return;

diff --git a/src/tools/gribjump-validate.cc b/src/tools/gribjump-validate.cc
@@ -92,7 +92,7 @@ void CompareEccodes::execute(const eckit::option::CmdArgs &args) {
     // Extract the data using gribjump
 
     GribJump gj;
-    std::vector<std::vector<ExtractionResult*>> results = gj.extract(polyRequest);
+    std::vector<std::vector<std::unique_ptr<ExtractionResult>>> results = gj.extract(polyRequest);
 
     ASSERT(results.size() == requests.size());
 

diff --git a/tests/remote/test_remote.cc b/tests/remote/test_remote.cc
@@ -58,7 +58,7 @@ CASE( "Remote protocol: extract" ) {
     }
 
     GribJump gribjump;
-    std::vector<std::vector<ExtractionResult*>> output = gribjump.extract(exRequests);
+    std::vector<std::vector<std::unique_ptr<ExtractionResult>>> output = gribjump.extract(exRequests);
 
     EXPECT_EQUAL(output.size(), 2);
     for (size_t i = 0; i < output.size(); i++) {

diff --git a/tests/test_api.cc b/tests/test_api.cc
@@ -36,7 +36,7 @@ namespace test {
 // Build expected values for test ranges
 constexpr double MISSING = std::numeric_limits<double>::quiet_NaN();
 
-void compareValues(const std::vector<std::vector<std::vector<std::vector<double>>>>& expectedValues, const std::vector<std::vector<ExtractionResult*>>& output) {
+void compareValues(const std::vector<std::vector<std::vector<std::vector<double>>>>& expectedValues, const std::vector<std::vector<std::unique_ptr<ExtractionResult>>>& output) {
     EXPECT(expectedValues.size() == output.size());
     for (size_t i = 0; i < expectedValues.size(); i++) { // each mars request
         EXPECT_EQUAL(expectedValues[i].size(), output[i].size());
@@ -136,7 +136,7 @@ CASE( "test_gribjump_api_extract" ) {
 
     // Extract values
     GribJump gj;
-    std::vector<std::vector<ExtractionResult*>> output1 = gj.extract(polyRequest1);
+    std::vector<std::vector<std::unique_ptr<ExtractionResult>>> output1 = gj.extract(polyRequest1);
 
     // Eccodes expected values
     std::vector<std::vector<std::vector<std::vector<double>>>> expectedValues;
@@ -163,7 +163,7 @@ CASE( "test_gribjump_api_extract" ) {
     PolyRequest polyRequest2;
     polyRequest2.push_back(ExtractionRequest(requests[0], ranges, gridHash));
 
-    std::vector<std::vector<ExtractionResult*>> output2 = gj.extract(polyRequest2);
+    std::vector<std::vector<std::unique_ptr<ExtractionResult>>> output2 = gj.extract(polyRequest2);
     EXPECT(output2.size() == 1);
     EXPECT(output2[0].size() == 3);
 
@@ -178,7 +178,7 @@ CASE( "test_gribjump_api_extract" ) {
     EXPECT_THROWS_AS(gj.extract({ExtractionRequest(requests[0], ranges, "wronghash")}), eckit::SeriousBug); // incorrect hash
 
     // correct hash
-    std::vector<std::vector<ExtractionResult*>> output2c = gj.extract({ExtractionRequest(requests[0], ranges, gridHash)});
+    std::vector<std::vector<std::unique_ptr<ExtractionResult>>> output2c = gj.extract({ExtractionRequest(requests[0], ranges, gridHash)});
     EXPECT_EQUAL(output2c[0][0]->total_values(), 15);
 
     // --------------------------------------------------------------------------------------------
@@ -216,13 +216,16 @@ CASE( "test_gribjump_api_extract" ) {
     EXPECT(outputItems3.size() == 3);
 
     /// @todo temp: convert to extractionResult
-    std::vector<ExtractionResult*> output3;
+    std::vector<std::unique_ptr<ExtractionResult>> output3;
     for (size_t i = 0; i < outputItems3.size(); i++) {
-        output3.push_back(new ExtractionResult(outputItems3[i]->values(), outputItems3[i]->mask()));
+        // output3.push_back(new ExtractionResult(outputItems3[i]->values(), outputItems3[i]->mask()));
+        output3.push_back(std::make_unique<ExtractionResult>(outputItems3[i]->values(), outputItems3[i]->mask()));
     }
 
     // Expect output to be the same as output2[0]
-    compareValues(expectedValues, {output3});
+    std::vector<std::vector<std::unique_ptr<ExtractionResult>>> output3v;
+    output3v.push_back(std::move(output3)); // i.e. == {output3}
+    compareValues(expectedValues, output3v);
 
     std::cout << "test_gribjump_api_extract got to the end" << std::endl;
 }