search

dmlc / dmlc-core

A common bricks library for building scalable and portable distributed machine learning.
Apache License 2.0
865 stars 519 forks source link

Move constructor is added to support dmlc::optional<T> #658

Closed mozga-intel closed 2 years ago

mozga-intel commented 3 years ago

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):

  1. constructor to construct an object that does not contain a value,
  2. constructor to construct an object that contains a nullptr value,
  3. constructs an optional object that contains a value, initialized as if direct-initializing:`
  4. move constructor: If other contains a value, then the stored value is direct-intialized with it. optional(optional &&other) noexcept() { }
  5. move constructor: constructs the stored value with value with otherparameter. optional(U &&other) noexcept { }

Upgrade: dmlc::optional value 1.T &value() &

  1. const T &value() const &
  2. T &&value() &
  3. const T && value() const &&
hcho3 commented 3 years ago

@szha @mozga-intel Do you still want my review?

mozga-intel commented 3 years ago

@hcho3 If you can review it, I'll be grateful. Thanks!

mozga-intel commented 3 years ago

Gentle ping @hcho3, @szha

mozga-intel commented 2 years ago

@szha, @hcho3 Thanks! There are a few things to improve: firstly, a constructor that can be declared with a constexpr specifier. Secondly, as @hcho3 mentioned, std::optional might be added to support it ~ it will be required a few changes in the code structure.

szha commented 2 years ago

@mozga-intel sounds good. Do you intend to continue in this PR or as a follow-up?

mozga-intel commented 2 years ago

@szha after merge! Because I need to test it before.

areusch commented 2 years ago

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 tests/scripts/task_config_build_cpu.sh) segfaults:

cd rust/tvm/examples/resnet
python3 src/build_resnet.py --build-dir=.

still tracing through to see why, but posting up here in case any of you all have ideas.

cc @tqchen