1

So im trying to build a merkle tree from a vector of strings, first convert all the strings to the MerkleNodes and then form their parents, building the tree upside down. But it seems to be an issue when i push all the nodes to the vector in the first FOR loop, because i cant access to them. I ve debug it and that seems to be the problem altough i dont know what else i can do, actually I first try with raw pointers but get the same issue.

  inline std::shared_ptr<MerkleNode> AddLeaf(const std::string &vright, const std::string &vleft) {
     std::shared_ptr<merkle::MerkleNode> right (new MerkleNode (vright));
     std::shared_ptr<merkle::MerkleNode> left (new MerkleNode (vleft));
     return std::make_shared<merkle::MerkleNode>(std::move(left), std::move(right));
 }

 std::shared_ptr<merkle::MerkleNode> Build(const std::vector<std::string> &values) {
     std::vector< std::shared_ptr<merkle::MerkleNode> > nodes(values.size());

     for (const auto &value: values) {
         nodes.push_back(std::make_shared<merkle::MerkleNode>(value));
     }
// I verify that the values are correct and that inmediatly after the push_back i can access to the nodes. 
//But in the second FOR loop with iterators i get ERROR -1073741819 (0xC0000005) when accessing the elemnts.
     std::vector< std::shared_ptr<merkle::MerkleNode> >::iterator it;
     while (nodes.size() > 1) {
         if (nodes.size() % 2 != 0) nodes.push_back(std::move(nodes.back()));
         std::vector<std::shared_ptr<merkle::MerkleNode> > new_nodes;

         for (it = nodes.begin(); it != nodes.end(); it += 2) {
             //std::shared_ptr<merkle::MerkleNode> h(it->get());

             auto temp = std::move(AddLeaf((*it)->hash(),(*(it+1))->hash()));
             new_nodes.push_back(std::move(temp));
         }
         //nodes = new_nodes;

     }
     return std::move(std::shared_ptr<MerkleNode>(nodes[0].get()));
 }
Mauricio Alvarez
  • 11
  • 1
  • 2
    `std::move(std::shared_ptr(nodes[0].get()));` is a recipe for death. The pointer from the ctor argument is already owned by one smart pointer control block; now you're standing it up in another. And your sizing on `nodes` is wrong anyway. `std::vector< std::shared_ptr > nodes(values.size());` creates a vector of `values.size()` empty smart pointers, then just pushes new ones on the *end* of that sequence. From what I see that should be `std::vector< std::shared_ptr > nodes;` – WhozCraig Oct 06 '22 at 04:49
  • `nodes.push_back(std::move(nodes.back()));` smells too. You are using move semantics instead of copying. – Ari0nhh Oct 06 '22 at 04:54
  • Mauricio there is a lot going on in the few lines you posted, I got really confused to the point of not being able to help. There are (1) pointers (2) moves (3) vector ops. I would suggest to give a step back and remove one of them. Let the shared pointer do its job and perhaps remove all std::move. If you were using unique_ptr you could not do this but with shared_ptr you can. You might get a performance penalty but you will remove one term of this equation. Once you figure out what's wrong, you can carefully put it back. – Something Something Oct 06 '22 at 06:24
  • [What is a debugger and how can it help me diagnose problems?](https://stackoverflow.com/questions/25385173/what-is-a-debugger-and-how-can-it-help-me-diagnose-problems) – Jesper Juhl Oct 06 '22 at 12:00
  • 1
    @HenriqueBucher You're right, i just get confuse and tried everything i saw on oter examples, i'll try to simplify and update! thanks – Mauricio Alvarez Oct 06 '22 at 14:22
  • Move semantics is more ideology than a real solution. It is optional. If you use shared pointers you do not need it anywhere. – Something Something Oct 06 '22 at 15:39

2 Answers2

0

Treat shared_ptr as a value type i.e. pass the pointer itself by copy, in order to propagate ownership, which is what this smart pointer is intended for.

Also, move shouldn't be used when returning because it interferes with compiler optimizations. The compiler knows that the return item should be moved into a new scope (return value optimization).

For more details, see this ISO CPP guideline about move: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es56-write-stdmove-only-when-you-need-to-explicitly-move-an-object-to-another-scope

Speaking for the code sample, you would probably benefit from moving large strings into new owners, if they're not used elsewhere after moving.

Andrei M.
  • 1
-5

Just use RAW pointers and FOR EACH, iterators and smart pointers just too much thinking...

Coding_in_india
  • 5
  • Your answer could be improved with additional supporting information. Please [edit] to add further details, such as citations or documentation, so that others can confirm that your answer is correct. You can find more information on how to write good answers [in the help center](/help/how-to-answer). – Community Oct 10 '22 at 06:48