Move constructor is added to support dmlc::optional<T> #658
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mozga-intel
force-pushed
the
mozga-intel/optional_move_support
branch
2 times, most recently
from
29211ba
to
fe6c4ea
Compare
6 tasks
mozga-intel
commented
Sep 14, 2021
6 tasks
sfraczek
approved these changes
Sep 17, 2021
mozga-intel
force-pushed
the
mozga-intel/optional_move_support
branch
from
9367bcb
to
46ad95b
Compare
mozga-intel
force-pushed
the
mozga-intel/optional_move_support
branch
from
b5ea461
to
fce1d9f
Compare
|
@szha @mozga-intel Do you still want my review? |
|
@hcho3 If you can review it, I'll be grateful. Thanks! |
szha
approved these changes
Nov 5, 2021
hcho3
approved these changes
Nov 5, 2021
|
@mozga-intel sounds good. Do you intend to continue in this PR or as a follow-up? |
|
@szha after merge! Because I need to test it before. |
|
not quite sure why yet, but it looks like when i apply this patch to TVM, the following command (in tvm repo, at rev c07a46327c86fc541297ebb985cc9c1dcef5a0db, using docker container tlcpack/ci-cpu:v0.84 and config.cmake generated with still tracing through to see why, but posting up here in case any of you all have ideas. cc @tqchen |
areusch
reviewed
Apr 21, 2022
| optional(dmlc::nullopt_t) noexcept : is_none(true) {} // NOLINT(*) | ||
| /*! \brief copy constructor, if other contains a value, then stored value is | ||
| * direct-intialized with it. */ | ||
| optional(const optional &other) = default; |
There was a problem hiding this comment.
it seems that reverting the use of the default copy constructor here stops the segfault
areusch
added a commit
to areusch/dmlc-core
that referenced
this pull request
Apr 28, 2022
areusch
added a commit
to areusch/dmlc-core
that referenced
this pull request
Apr 28, 2022
areusch
added a commit
to areusch/dmlc-core
that referenced
this pull request
Apr 28, 2022
tqchen
pushed a commit
that referenced
this pull request
Apr 29, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The class template std::optional manages an optional contained value, i.e. a value that may or may not be present. In this pull request, a few things were implemented, such as extended series of constructors and a function of value. Mainly, this pull request is a realization of changes that have an impact on constructors, as follows, (upgrade dmlc::optional constructors):
optional(optional &&other) noexcept() { }otherparameter.optional(U &&other) noexcept { }Upgrade: dmlc::optional value
1.
T &value() &2.
const T &value() const &3.
T &&value() &4.
const T && value() const &&