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

CoverTree and BinarySpaceTree Segmentation Fault during destruction #254

Closed
rcurtin opened this issue Dec 29, 2014 · 2 comments
Closed

CoverTree and BinarySpaceTree Segmentation Fault during destruction #254

rcurtin opened this issue Dec 29, 2014 · 2 comments
Assignees
Milestone

Comments

@rcurtin
Copy link
Member

rcurtin commented Dec 29, 2014

Reported by trironk3 on 1 Oct 42975421 15:49 UTC
I'm encountering a Segmentation Fault during the destruction of CoverTree and BinarySpaceTree. I'm attaching my test code.

@rcurtin rcurtin self-assigned this Dec 29, 2014
@rcurtin rcurtin added this to the mlpack 1.0.4 milestone Dec 29, 2014
@rcurtin rcurtin closed this as completed Dec 29, 2014
@rcurtin
Copy link
Member Author

rcurtin commented Dec 30, 2014

Commented by rcurtin on 9 Aug 42975558 15:28 UTC
The issue was that PrefixedOutStream::operator<<(T) was generating copies. Those objects which were segfaulting are those which do not have proper copy constructors.

The key change is this:

PrefixedOutStream::operator<<(T)

to this:

PrefixedOutStream::operator<<(const T&)

If we were to change it to simply 'T&' and not 'const T&', this would not work for temporaries (i.e. Log::Warn << Range(5, 6);) because temporaries are not functionally equivalent to references. However, temporaries are functionally equivalent to const references, so changing the signature to 'const T&' avoids copies while at the same time not adversely impacting functionality. This change is committed in r14043.

However, the ticket's not quite done yet. Those offending objects (MRKDStatistic, HRectBound, BinarySpaceTree, CoverTree, this and that) should at least have properly-written copy constructors. Otherwise a user will accidentally copy them and then everything goes to hell at destruction time.

@rcurtin
Copy link
Member Author

rcurtin commented Dec 30, 2014

Commented by rcurtin on 4 Mar 42975802 17:56 UTC
I added copy constructors to BinarySpaceTree and CoverTree. So we can call this good for now, then.

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

No branches or pull requests

1 participant