Skip to content

Commit

Permalink
[DF] Remove RResultPtr::Release (an unreleased feature)
Browse files Browse the repository at this point in the history
The current implementation is broken (fObjPtr.release() deletes the
object thet we then return by pointer) and in general it is extremely
awkward to convince a std::shared_ptr to release ownership (and for good
reason, it only makes sense if you can be absolutely sure that there is
only one shared_ptr alive that owns that object).

Instead of using RResultPtr::Release, we recommend moving the result
out of the RResultPtr.

This effectively reverts commit 03fd58e.
  • Loading branch information
eguiraud committed Mar 12, 2021
1 parent 46ef2a3 commit b802a6b
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 28 deletions.
12 changes: 0 additions & 12 deletions tree/dataframe/inc/ROOT/RResultPtr.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -326,18 +326,6 @@ public:
return *this;
}

/// Return a pointer to the result, releasing its ownership and leaving this object empty.
T *Release()
{
if (fActionPtr != nullptr && !fActionPtr->HasRun())
TriggerRun();
fActionPtr = nullptr;
fLoopManager = nullptr;
auto p = fObjPtr.get();
fObjPtr.reset();
return p;
}

// clang-format off
/// Check whether the result has already been computed
///
Expand Down
18 changes: 2 additions & 16 deletions tree/dataframe/test/dataframe_resptr.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -52,25 +52,14 @@ TEST(RResultPtr, MoveCtor)
EXPECT_EQ(*res, 1u);
}

TEST(RResultPtr, Release)
{
ROOT::RDataFrame df(1);
auto p = df.Sum<ULong64_t>("rdfentry_");
p.GetValue();
p.Release();
EXPECT_TRUE(p == nullptr);
}

TEST(RResultPtr, NullResultPtr)
{
// build null result ptrs in 3 different ways
// build null result ptr
ROOT::RDF::RResultPtr<TH1D> r1;

// set result ptr to null with a move
auto r2 = ROOT::RDataFrame(1).Histo1D<ULong64_t>("rdfentry_");
auto r3 = r2;
ROOT::RDF::RResultPtr<TH1D>(std::move(r2));
EXPECT_EQ(r3->GetEntries(), 1ll); // trigger event loop, to check moved-after-event-loop state
r3.Release();

// make sure they have consistent, sane behavior
auto checkResPtr = [](ROOT::RDF::RResultPtr<TH1D> &r) {
Expand All @@ -83,13 +72,10 @@ TEST(RResultPtr, NullResultPtr)
EXPECT_THROW(r.OnPartialResult(1, [] (TH1D&) {}), std::runtime_error);
EXPECT_THROW(r->GetEntries(), std::runtime_error);
EXPECT_THROW(*r, std::runtime_error);

EXPECT_EQ(r.Release(), nullptr);
};

checkResPtr(r1);
checkResPtr(r2);
checkResPtr(r3);
}

TEST(RResultPtr, ImplConv)
Expand Down

0 comments on commit b802a6b

Please sign in to comment.