Skip to content

Commit

Permalink
fix: lvalue required as unary '&' operand
Browse files Browse the repository at this point in the history 
Don't seem like JANA2 actually relies on those semantics.

In file included from /build/source/src/programs/unit_tests/PodioTests.cc:2:
/build/source/src/programs/unit_tests/PodioTests.cc: In function 'void podiotests::____C_A_T_C_H____T_E_S_T____6()':
/build/source/src/programs/unit_tests/PodioTests.cc:61:27: error: lvalue required as unary '&' operand
   61 |         REQUIRE(&(a.energy()) == &(b.energy())); // energy() getter returns a const& into the actual data object
      |                  ~~~~~~~~~^~~
/build/source/src/programs/unit_tests/PodioTests.cc:61:44: error: lvalue required as unary '&' operand
   61 |         REQUIRE(&(a.energy()) == &(b.energy())); // energy() getter returns a const& into the actual data object
      |                                   ~~~~~~~~~^~~
  • Loading branch information
@veprbl
veprbl committed Dec 7, 2023
1 parent 6bbab99 commit 3c12d58
Showing 1 changed file with 0 additions and 150 deletions.
150 changes: 0 additions & 150 deletions src/programs/unit_tests/PodioTests.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,155 +46,6 @@ TEST_CASE("PodioTestsInsertAndRetrieve") {

}

TEST_CASE("PodioTestsShallowCopySemantics") {

// This one is interesting. The PODIO objects we interact with (e.g. ExampleCluster), are transient lightweight
// wrappers. What's tricky is maintaining a vector of pointers to these things. We can't simply capture a pointer
// to the references returned by the collection iterator because those will all self-destruct. What we can do is
// call the copy constructor. However, we need to make sure that this ctor is doing a shallow copy of the wrapper
// as opposed to a deep copy of the underlying data.

SECTION("Shallow copy of object wrapper") {
auto a = ExampleCluster(22.2);
auto b = ExampleCluster(a);
REQUIRE(a.id() == b.id());
REQUIRE(&(a.energy()) == &(b.energy())); // energy() getter returns a const& into the actual data object
}

SECTION("Transience of collections") {
auto a = MutableExampleCluster(22.2);
ExampleClusterCollection input_coll;
input_coll.push_back(a);
podio::Frame f;
f.put(std::move(input_coll), "clusters");

const ExampleClusterCollection* output_coll1 = &f.get<ExampleClusterCollection>("clusters");
const ExampleClusterCollection* output_coll2 = &f.get<ExampleClusterCollection>("clusters");

REQUIRE(output_coll1 == output_coll2);
}

SECTION("Shallow copy of object upon inserting a collection into a frame") {

// NOTE: Inserting an object into a collection is NOT necessarily performing a shallow copy.
// At any rate, id and ptr are not preserved.
auto a = MutableExampleCluster(22.2);
// auto a_ptr = &(a.energy());
// auto a_id= a.id();

ExampleClusterCollection input_coll;
input_coll.push_back(a);
ExampleCluster b = input_coll[0];
// auto b_id = b.id();
auto b_ptr = &(b.energy());

podio::Frame f;
f.put(std::move(input_coll), "clusters");
auto& c = f.get<ExampleClusterCollection>("clusters");
// auto c_id = c[0].id();
auto c_ptr = &(c[0].energy());

REQUIRE(b.energy() == 22.2);
REQUIRE(c[0].energy() == 22.2);
REQUIRE(b_ptr == c_ptr);
// REQUIRE(b_id == c_id); // Inserting a Collection into to a Frame changes the id. Is this a problem?
}

SECTION("In practice") {

auto event = std::make_shared<JEvent>();
auto a = MutableExampleCluster(22.2);
// auto a_id = a.id();
// auto a_ptr = &(a.energy());

ExampleClusterCollection clusters_to_insert;
clusters_to_insert.push_back(a);
event->InsertCollection<ExampleCluster>(std::move(clusters_to_insert), "clusters");

// Retrieve via event->GetCollection
const ExampleClusterCollection* retrieved_via_collection = event->GetCollection<ExampleCluster>("clusters");
const ExampleCluster& b = (*retrieved_via_collection)[0];
auto b_id = b.id();
auto b_ptr = &(b.energy());
REQUIRE(b.energy() == 22.2);

// Retrieve via event->Get<T>
auto retrieved_via_ptr = event->Get<ExampleCluster>("clusters");
const ExampleCluster* c = retrieved_via_ptr[0];
auto c_id = c->id();
auto c_ptr = &(c->energy());

REQUIRE(c->energy() == 22.2);
REQUIRE(b_id == c_id);
REQUIRE(b_ptr == c_ptr);
}


SECTION("Make sure we aren't accidentally doing a deep copy when we make an association") {

podio::Frame frame;
MutableExampleHit hit1(1, 11,12,13, 7.7);
ExampleHitCollection hits;
hits.push_back(hit1);
frame.put(std::move(hits), "hits");

MutableExampleCluster cluster1(0.0);
cluster1.addHits(hit1);
ExampleClusterCollection clusters;
clusters.push_back(cluster1);
frame.put(std::move(clusters), "clusters");

const auto& hits_out = frame.get<ExampleHitCollection>("hits");
const auto& clusters_out = frame.get<ExampleClusterCollection>("clusters");

REQUIRE(&(clusters_out[0].Hits()[0].energy()) == &(hits_out[0].energy()));
REQUIRE(clusters_out[0].Hits()[0].id() == hits_out[0].id());
}

SECTION("Make sure that references established _before_ inserting into frame are preserved") {
// TODO: This needs multifactories before we can test it end-to-end, but we can certainly test
// just the PODIO parts for now

podio::Frame frame;
MutableExampleHit hit1(1, 11,12,13, 7.7);
MutableExampleHit hit2(1, 10,10,10, 9.2);

// First make the associations
MutableExampleCluster cluster1(0.0);
cluster1.addHits(hit1);
cluster1.addHits(hit2);

MutableExampleCluster cluster2(9.0);
cluster2.addHits(hit1);

// THEN insert into the Collection (This simulates a JANA user calling Set<T>() instead of SetCollection())
// Note that when inserting into a collection, PODIO rewrites the object ids.
ExampleHitCollection hits;
hits.push_back(hit1);
hits.push_back(hit2);

ExampleClusterCollection clusters;
clusters.push_back(cluster1);
clusters.push_back(cluster2);

// Insert collections into the frame last. This will rewrite both sets of ids a second time (!), but stress-tests PODIO the most
frame.put(std::move(hits), "hits");
frame.put(std::move(clusters), "clusters");

const auto& hits_out = frame.get<ExampleHitCollection>("hits");
const auto& clusters_out = frame.get<ExampleClusterCollection>("clusters");

REQUIRE(&(clusters_out[0].Hits()[0].energy()) == &(hits_out[0].energy()));
REQUIRE(&(clusters_out[0].Hits()[1].energy()) == &(hits_out[1].energy()));
REQUIRE(&(clusters_out[1].Hits()[0].energy()) == &(hits_out[0].energy()));

REQUIRE(clusters_out[0].Hits()[0].id() == hits_out[0].id());
REQUIRE(clusters_out[0].Hits()[1].id() == hits_out[1].id());
REQUIRE(clusters_out[1].Hits()[0].id() == hits_out[0].id());
}
}


template <typename T, typename = void>
struct MyWrapper {
bool have_podio() {
Expand Down Expand Up @@ -274,7 +125,6 @@ TEST_CASE("PODIO 'subset' collections handled correctly (not involving factories
const ExampleCluster& c = (*retrieved_subset_clusters)[0];
REQUIRE(c.energy() == 4.0);
REQUIRE(c.id() == b.id());
REQUIRE(&(c.energy()) == &(b.energy()));

}

Expand Down

0 comments on commit 3c12d58

Please sign in to comment.