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

For Node<T> move constructor, T copy constructor is called when the move constructor is not defined but the move assignment operator is defined. #345

Closed
AryanGitHub opened this issue Sep 19, 2023 · 1 comment · Fixed by #346
Labels
bug Something isn't working core something about core performance Performance issue Priority:Critical Priority Label for Critical Issue

Comments

@AryanGitHub
Copy link
Contributor

Describe the bug
For Node<T>, when Node<T>::Node(const std::string& id, T&& data) is invoked for T, where T's move constructor is not defined but T's move assignment operator is defined, the T's copy constructor is invoked.
Hence where a move operation can be used, a copy operation is used.

A move operation is of constant time complexity, whereas a copy operation is of linear time complexity

To Reproduce (Minimal Reproducable Example)
Steps to reproduce the behavior:

  1. Compile the given below code.
#include <CXXGraph.hpp>

#include <memory>

struct obj {
    public:
    std::string str;

    obj()
    {
        std::cout << "default constructor used" << std::endl;
    }
    obj(std::string input_str)
        : str (input_str)
    {
        std::cout << "parameterised constructor used" << std::endl;
    }

    obj(const obj& copySource)
    {
        this->str = copySource.str;
        std::cout << "copy constructor" << std::endl;
    }
    /*
    obj(obj&& moveSource)
    {
        this->str = std::move(moveSource.str);
        std::cout << "move constructor" << std::endl;
    }*/

    void operator= (const obj& source) 
    {
        std::cout << "copy assignment operator called" << std::endl;
        this->str = source.str;
    }
    
    void operator= (obj&& source)
    {
        std::cout << "move assignment operator called" << std::endl;
        this->str = std::move(source.str);
    }
    ~obj()
    {
        std::cout << "destructor called" << std::endl;
    }
};

int main() {
  
  CXXGraph::Node<obj> obj_node_0("obj_node_0" ,obj("Hello,World!"));
  return 0;
}
  1. Output displays the usage of the copy constructor

Observed behavior
The output uses a copy constructor.

parameterised constructor used
default constructor used
copy constructor
move assignment operator called
move assignment operator called
destructor called
destructor called
destructor called

Expected behavior
The output should only use the move assignment operator.

parameterised constructor used
default constructor used
move assignment operator called
destructor called
destructor called

Reason it happened
in Node<T>::Node(const std::string& id, T&& data) a std::swap() is used.
According to cpprefrence, std::swap() uses move constructor and move assignment operator both.
So when move constructor is not defined, it shifts back to copy constructor.
hence, defeating the purpose of move.

How to resolve?
Use std::move() in Node<T>::Node(const std::string& id, T&& data) instead, so even only if move assignment operator is defined, move can take place.

Thanks.

@ZigRazor ZigRazor added bug Something isn't working core something about core Priority:Critical Priority Label for Critical Issue performance Performance issue labels Sep 19, 2023
@ZigRazor
Copy link
Owner

Thank you for the opened Issue. We try to resolve it as soon as possible

AryanGitHub added a commit to AryanGitHub/CXXGraph that referenced this issue Sep 19, 2023
 used std::move() in Node<T> move constructor
ZigRazor pushed a commit that referenced this issue Sep 20, 2023
used std::move() in Node<T> move constructor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core something about core performance Performance issue Priority:Critical Priority Label for Critical Issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants