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

- make test fails to find testxgboost, if a build directory is created #4301

Merged
merged 5 commits into from
Mar 31, 2019

Conversation

sriramch
Copy link
Contributor

on which cmake is run. this appends the build directory to the test binary

  • make the assignments of HostDeviceVector exception safe
  • rm storing a dummy GPUDistribution instance for CPU based code

  on which cmake is run. this appends the build directory to the test binary
- make the assignments of HostDeviceVector exception safe
- rm storing a dummy GPUDistribution instance for CPU based code
@rongou
Copy link
Contributor

rongou commented Mar 27, 2019

@RAMitchell

@sriramch
Copy link
Contributor Author

#3720 reference to bad_alloc

@codecov-io
Copy link

codecov-io commented Mar 28, 2019

Codecov Report

Merging #4301 into master will increase coverage by <.01%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4301      +/-   ##
==========================================
+ Coverage   67.82%   67.82%   +<.01%     
==========================================
  Files         132      132              
  Lines       12202    12203       +1     
==========================================
+ Hits         8276     8277       +1     
  Misses       3926     3926
Impacted Files Coverage Δ
src/common/host_device_vector.h 75% <ø> (ø) ⬆️
src/common/host_device_vector.cc 64.47% <85.71%> (+0.96%) ⬆️
src/tree/updater_histmaker.cc 64.62% <0%> (-0.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d5b34d...f800657. Read the comment docs.

@trivialfis trivialfis self-requested a review March 30, 2019 14:00
Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sriramch Your assignment operator is not exception safe since new can throw. https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom might help a little. But I would leave it to another PR. ;-)

@sriramch
Copy link
Contributor Author

@sriramch Your assignment operator is not exception safe since new can throw. https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom might help a little. But I would leave it to another PR. ;-)

@trivialfis what this patch is attempting to do is that when a target hostvectorimpl (other) is assigned to a source (*this), any intermediary exception that may occur during this assignment does not alter the initial state of the source. it is not trying to make the assignment operator not throw any exceptions at all. this entails simply doing the copy work on the side and when that operation is deemed successful, simply swaps the guts of it with the source. if the side work fails ofr some reason (say due to new throwing as you state), source object (*this) isn't impacted in any way

Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sriramch Ah, I see. Thanks for the explanation. Looks good to me.

@trivialfis trivialfis merged commit 2f7087e into dmlc:master Mar 31, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants