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

CollectionBase::getBuffers should be marked const #302

Open
tmadlener opened this issue Jun 14, 2022 · 3 comments
Open

CollectionBase::getBuffers should be marked const #302

tmadlener opened this issue Jun 14, 2022 · 3 comments

Comments

@tmadlener
Copy link
Collaborator

Once the Frame is merged and used CollectionBase::getBuffers should be marked const as it will be required for writing from multiple threads. In principle it should be rather straight forward to do.

This will only become possible once #287 is merged, because currently getBuffers is also used for reading and not only writing.

@hegner
Copy link
Collaborator

hegner commented Jul 13, 2023

For this the handling of the UserDataCollections would need to change. Any plans for that?

@tmadlener
Copy link
Collaborator Author

There are effectively two options as I see it at the moment:

  • Mark _vecPtr as mutable, such that we can still assign in a const function. This would potentially require to also introduce a mutex into the UserDataCollection to protect against race conditions.
  • Give up on default move-ability, and properly implement non-default constructors and move-constructors that set _vecPtr during initialization, such that a const getBuffers becomes trivial.

I am leaning towards the second here (although I think I have a branch somewhere for the first), simply because I don't want the mutex in there and, I think we still need a const_cast with that.

@tmadlener
Copy link
Collaborator Author

After having a look into this, I realized this will require quite a bit of work and we decided to address this only after a v1.0 release, since it is not really user visible in any case (or at least it shouldn't be). Currently we explicitly const_cast the collections in the writers, e.g.:

auto* coll = frame.getCollectionForWrite(name);
collections.emplace_back(name, const_cast<podio::CollectionBase*>(coll));

Addressing this properly also requires changes in the CollectionWriteBuffers since they currently store non-const pointers, but if we mark getBuffers as const we would need a constructor from const pointers. The CollectionWriteBuffers should definitely only have const pointers, but to get things to that requires a bit of time.

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

No branches or pull requests

2 participants