AIDASoft / podio

PODIO
GNU General Public License v3.0
24 stars 60 forks source link

Changes to dereferenced collection iterators are not reflected in the underlying collection #281

Open tmadlener opened 2 years ago

tmadlener commented 2 years ago

I have to point out that being able to compile code that has incorrect results is already there in master. This PR doesn't introduce anything new in that sense (though I agree that it may increase expectations among users that some algorithms will work when they don't, and that we should fail to compile for things that don't work). Using the current master branch the following fairly straightforward example compiles but fails to do what was intended:

  auto collection = ExampleClusterCollection();
  collection.create();
  for (auto i = collection.begin(); i != collection.end(); ++i) {
    *i = MutableExampleCluster(42);
  }
  REQUIRE(collection.begin()->energy() == 42);

Originally posted by @wdconinc in https://github.com/AIDASoft/podio/issues/273#issuecomment-1090493117

tmadlener commented 2 years ago

Just to add a few examples that also do not work as expected:

auto collection = ExampleClusterCollection();
collection.create(); // energy == 0.0 by default

for (auto&& c : collection) {
  c = MutableExampleCluster(42);
  REQUIRE(c.energy() == 42);
}
REQUIRE(collection[0].energy() == 42); // fails

collection[0] = MutableExampleCluster(42);
REQUIRE(collection[0].energy() == 42); // fails

The loop example is in a sense the same as above. It does however at least give a potential hint to users, as for (auto& c : collection) (single ampersand) does not compile and results in the following error:

../tests/unittest.cpp:345:18: error: cannot bind non-const lvalue reference of type ‘MutableExampleCluster&’ to an rvalue of type ‘MutableExampleCluster’
  345 |   for (auto& c : collection) {
      |                  ^~~~~~~~~~

Nevertheless the indexing example is a problem as that looks like it should work and simply doesn't silently.

andresailer commented 2 years ago