<p>Hi <a href="https://github.com/rcurtin" class="user-mention">@rcurtin</a>, thanks for your comments.<br>
I have update the code to include your suggestions:</p>

<ul>
<li>I have added more overloads for the constructor and Train() methods, to consider lvalue and rvalue references (copy and move semantics).
All of them are very simple because they just create a new tree and then call the overloads that take <code>TreeType*</code>.</li>
<li>I didn't remove the overlads that take <code>TreeType*</code> to maintain reverse compatibility, as you suggested. If you agree, we could add a comment that these methods will be removed in the future release (if we decide that).
In my opinion we should remove that overloads in the next release, and only maintain the 2 overloads with references. If we do so, we will sure that <code>NeighborSearch</code> class always has ownership of the reference tree (because it was copied or moved).</li>
</ul>

<p>Would you agree?</p>

<p>Thanks!</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">&mdash;<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/mlpack/mlpack/pull/743#issuecomment-239550128">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AJ4bFIZLOG55LV28kci0TJmYp7iJhTo9ks5qfNWTgaJpZM4JXoXp">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/AJ4bFCyZwzaKcnQxFdkXCd3Ikmqjc6d1ks5qfNWTgaJpZM4JXoXp.gif" width="1" /></p>
<div itemscope itemtype="http://schema.org/EmailMessage">
<div itemprop="action" itemscope itemtype="http://schema.org/ViewAction">
  <link itemprop="url" href="https://github.com/mlpack/mlpack/pull/743#issuecomment-239550128"></link>
  <meta itemprop="name" content="View Pull Request"></meta>
</div>
<meta itemprop="description" content="View this Pull Request on GitHub"></meta>
</div>

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/mlpack/mlpack","title":"mlpack/mlpack","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/mlpack/mlpack"}},"updates":{"snippets":[{"icon":"PERSON","message":"@MarcosPividori in #743: Hi @rcurtin, thanks for your comments.\r\nI have update the code to include your suggestions:\r\n+ I have added more overloads for the constructor and Train() methods, to consider lvalue and rvalue references (copy and move semantics).\r\nAll of them are very simple because they just create a new tree and then call the overloads that take `TreeType*`.\r\n+ I didn't remove the overlads that take `TreeType*` to maintain reverse compatibility, as you suggested. If you agree, we could add a comment that these methods will be removed in the future release (if we decide that).\r\nIn my opinion we should remove that overloads in the next release, and only maintain the 2 overloads with references. If we do so, we will sure that `NeighborSearch` class always has ownership of the reference tree (because it was copied or moved).\r\n\r\nWould you agree?\r\n\r\nThanks!"}],"action":{"name":"View Pull Request","url":"https://github.com/mlpack/mlpack/pull/743#issuecomment-239550128"}}}</script>