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

(Non)-Copyable Containers #111

Closed
hegner opened this issue Jul 6, 2020 · 8 comments · Fixed by #154
Closed

(Non)-Copyable Containers #111

hegner opened this issue Jul 6, 2020 · 8 comments · Fixed by #154

Comments

@hegner
Copy link
Collaborator

hegner commented Jul 6, 2020

Can we make containers non-copyable?

(a follow up on #109)

@hegner hegner mentioned this issue Jul 6, 2020
7 tasks
@joequant
Copy link
Contributor

joequant commented Jul 6, 2020

It looks promising. I ran into the same root issues you did, but it looks like that it's because something expects proxy classes that need copy constructors. It looks like once I understand the code a bit more, I can do something that can work around the problem.

@andresailer
Copy link
Member

For why the write command gives warnings, see the comment in the test definition:

add_test(NAME write COMMAND write)
# The following directories need to be added to the LD_LIBRARY_PATH:
# - CMAKE_CURRENT_BINARY_DIR: contains the root dictionary and pcm files for TestDataModel
# - CMAKE_BINARY_DIR/src: contains the root dictionary and pcm files for podio
set_property(TEST write PROPERTY ENVIRONMENT LD_LIBRARY_PATH=${CMAKE_CURRENT_BINARY_DIR}:${CMAKE_BINARY_DIR}/src:$ENV{LD_LIBRARY_PATH})

@joequant
Copy link
Contributor

joequant commented Jul 6, 2020

Got it. I've gotten everything to work in the base case and things to fail in the non-copyable case. I'll spend a few days looking at the error messages to give some better diagnostics when things are broken.

@joequant
Copy link
Contributor

Took a quick look. What the problem seems to be is that ROOT contains generic templates to convert collection classes to and from STL vectors. Once you mark the copy constructor as non-existent that fails. Also, it really should fail, since if the object is really non-copyable then bad things will happen once you copy it.

Right now, I'm digging into the code to see if I can override the behavior and cause it to throw an exception if you try to do a copy. It looks possible, but it all involves giving out the right C++magic.

@joequant
Copy link
Contributor

Okay here is the problem.....

  instance.AdoptCollectionProxyInfo(TCollectionProxyInfo::Generate(TCollectionProxyInfo::Pushback< vector<ex42::ExampleWithNamespaceData> >()));

The system tries to create proxies for vectors of containers, and vectors inheritly require copy constructors to put items into the vector.

It's possible to have vectors without copies using emplace_back, but not sure how this is going to work.

Ideas?

@joequant
Copy link
Contributor

I think the easy thing to do is to disable the creation of vectors of collections, since it doesn't make sense to have a collection of collections. Does that work?

joequant added a commit to joequant/podio that referenced this issue Nov 24, 2020
This disables the explicit assignment operator to enforce copy
constraints.  The copy constructor is not disabled since this
interferes with root adapters.
@tmadlener
Copy link
Collaborator

Because I just ran into a related problem myself:

void doSomething(const WhateverCollection collection);

vs the probably intended

void doSomething(const WhateverCollection& collection);

Both compile fine, but the former fails with a rather cryptic error message during runtime:

malloc_consolidate(): invalid chunk size

So, I think having truly non-copyable collections would be extremely nice to catch such things early.

@tmadlener
Copy link
Collaborator

I think the easy thing to do is to disable the creation of vectors of collections, since it doesn't make sense to have a collection of collections. Does that work?

I think that is a sensible suggestion. And if we remove that requirement we can actually remove the copy constructor and the copy assignment operator for collections. Currently we require that a vector<Collection> is streamable with root in the selection.xml that we generate. However, as far as I can tell, this is something we really do not need, since the only things we want to store in root files is vector<Data>, since each collection goes into its own branch in any case. It is possible to significantly cut down on the contents of the selection.xml, such that in the end only the

          <class name="{{classname}}Data"/>
          <class name="{{classname}}Collection"/>

are needed for all the generated classes. Interestingly the latter is necessary for a working python interface when building with gcc, but can be omitted when building with clang (and still have working python interface).

@tmadlener tmadlener mentioned this issue Jan 28, 2021
5 tasks
andresailer pushed a commit that referenced this issue Feb 2, 2021
This disables the explicit assignment operator to enforce copy
constraints.  The copy constructor is not disabled since this
interferes with root adapters.
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

Successfully merging a pull request may close this issue.

4 participants