Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
tmadlener opened this issue Apr 7, 2022 · 2 comments
Labels

Comments

@tmadlener
Copy link
Collaborator

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 #273 (comment)

@tmadlener tmadlener added the bug label Apr 7, 2022
@tmadlener
Copy link
Collaborator Author

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
Copy link
Member

  • Need to discuss what we want to support
    • What should actually happen when we assign a new handle to an existing one?
    • Things that look reasonable should work as expected.
  • Everything that doesn't work should fail at compile time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants