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

Confused about move to non-existing subtree #1178

Closed
robertsawko opened this issue Oct 26, 2023 · 5 comments · Fixed by #1179
Closed

Confused about move to non-existing subtree #1178

robertsawko opened this issue Oct 26, 2023 · 5 comments · Fixed by #1179
Labels

Comments

@robertsawko
Copy link

robertsawko commented Oct 26, 2023

Hi,

I was using node move earlier today in the code which can be represented by the following MVP:

#include <conduit.hpp>

int main (int argc, char *argv[]) {
    
    conduit::Node root;
    double* ptr = new double[3]{1, 2, 3};
    conduit::Node subtree;
    subtree["to/data"].set_external(ptr);
    root["path"].move(subtree);
    root.print();
    std::cout << "Node name: " << root["path"].name() << std::endl;
    return 0;
}

I am a bit confused by this. On one side, I feel it shouldn't work at all as "path" does not exist when I am moving the tree. Printing out the structure though confirms that it was created as intended. When asking explicitly about the name results in an empty string. Can you please advise, what's happening here?

Also, if I use a copy constructor

conduit::Node copy = root;
std::cout << "Node name after copy: " << copy["path"].name() << std::endl;

I get an intended "path" as output. So only the original now has a missing node name.

@cyrush
Copy link
Member

cyrush commented Oct 26, 2023

@robertsawko

Thanks for the question, executing:

root["path"]

Will create an empty node named "path"` as a child for root node - before the move is executed.

The move destination is the node it is called on, so in this case it is root["path"] instead of root

I would expect:

std::cout << "Node name: " << root["path"].name() << std::endl;

To show path, since this did not work- sounds like a bug & we need to fix the move implementation.

We will look into this more.

@cyrush cyrush added the bug label Oct 26, 2023
@robertsawko
Copy link
Author

Thanks for quick reply. I've stumbled across it randomly as I was trying to avoid copy constructors.

Just to confirm:

#include <conduit.hpp>

int main (int argc, char *argv[]) {
    conduit::Node root;

    double* ptr = new double[3]{1, 2, 3};
    conduit::Node subtree;
    subtree["to/data"].set_external(ptr, 3);
    root["path"].move(subtree);
    root["empty"];
    root.print();
    std::cout << "Node name: " << root["path"].name() << std::endl;
    std::cout << "Node name: " << root["empty"].name() << std::endl;
    conduit::Node copy = root;
    std::cout << "Node name after copy: " << copy["path"].name() << std::endl;
    return 0;
}

Gives on Conduit 0.8.8:

path:
  to:
    data: [1.0, 2.0, 3.0]
empty:

Node name:
Node name: empty
Node name after copy: path

@cyrush
Copy link
Member

cyrush commented Oct 26, 2023

thanks for the output -- we will work to fix this case.

@JustinPrivitera
Copy link
Member

JustinPrivitera commented Oct 26, 2023

Here is a simpler reproducer - it has to do with Node::swap():

Node root, subtree;
subtree["to/data"] = 5;
root["path"]["to"]["data"] = 6;


std::cout << "Node name: " << root["path"].name() << std::endl;
if (root["path"].schema().parent() != NULL)
    std::cout << "path schema parent exists" << std::endl;
else
    std::cout << "path schema parent does not exist" << std::endl;

// swap gets rid of the schema parent, which breaks name()
root["path"].swap(subtree);

std::cout << "Node name: " << root["path"].name() << std::endl;
if (root["path"].schema().parent() != NULL)
    std::cout << "path schema parent exists" << std::endl;
else
    std::cout << "path schema parent does not exist" << std::endl;

yields:

Node name: path
path schema parent exists
Node name: 
path schema parent does not exist

swap is used internally by move.

@JustinPrivitera
Copy link
Member

here is a fix: #1179

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

Successfully merging a pull request may close this issue.

3 participants