Make Ray code C++ compatible by pcmoritz · Pull Request #321 · ray-project/ray

pcmoritz · 2017-02-26T21:42:15Z

No description provided.

AmplabJenkins · 2017-02-28T19:31:49Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/134/
Test FAILed.

AmplabJenkins · 2017-02-28T23:00:47Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/139/
Test FAILed.

AmplabJenkins · 2017-03-01T02:40:21Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/142/
Test FAILed.

AmplabJenkins · 2017-03-01T03:44:31Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/145/
Test FAILed.

AmplabJenkins · 2017-03-01T03:55:51Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/146/
Test FAILed.

AmplabJenkins · 2017-03-01T04:30:32Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/147/
Test FAILed.

robertnishihara · 2017-03-01T04:40:41Z

Some compiler warnings

/Users/rkn/Workspace/ray/src/common/test/io_tests.cc:18:23: warning: ISO C++11 does not allow
      conversion from string literal to 'char *' [-Wwritable-strings]
  char *test_string = "hello world";
                      ^
/Users/rkn/Workspace/ray/src/common/test/io_tests.cc:19:22: warning: ISO C++11 does not allow
      conversion from string literal to 'char *' [-Wwritable-strings]
  char *test_bytes = "another string";
                     ^
/Users/rkn/Workspace/ray/src/common/test/io_tests.cc:63:22: warning: ISO C++11 does not allow
      conversion from string literal to 'char *' [-Wwritable-strings]
  char *test_bytes = "another string";
                     ^
3 warnings generated.

AmplabJenkins · 2017-03-01T04:48:14Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/148/
Test FAILed.

AmplabJenkins · 2017-03-01T04:57:51Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/149/
Test FAILed.

atumanov · 2017-03-01T05:00:31Z

    void *user_context) {
  CHECK(db_handle != NULL);
-  ObjectTableSubscribeData *sub_data = malloc(sizeof(ObjectTableSubscribeData));
+  ObjectTableSubscribeData *sub_data = (ObjectTableSubscribeData *) malloc(sizeof(ObjectTableSubscribeData));


I would put the entire right hand side on the second line .

atumanov · 2017-03-01T05:01:25Z

                           object_info_done_callback done_callback,
                           void *user_context) {
-  ObjectInfoSubscribeData *sub_data = malloc(sizeof(ObjectInfoSubscribeData));
+  ObjectInfoSubscribeData *sub_data = (ObjectInfoSubscribeData *) malloc(sizeof(ObjectInfoSubscribeData));


same here, might be easier to have the cast and the malloc on the same (second) line.

atumanov · 2017-03-01T05:02:24Z

 /* Including hiredis here is necessary on Windows for typedefs used in ae.h. */
 #include "hiredis/hiredis.h"
 #include "hiredis/adapters/ae.h"
+}


do we still need the extern C here?

Yes, we do not convert hiredis to C++

atumanov · 2017-03-01T05:12:02Z


 int wait_timeout_handler(event_loop *loop, timer_id id, void *context) {
-  wait_request *wait_req = context;
+  wait_request *wait_req = (wait_request *) context;


seems we're still far from being consistent with the new naming convention, but this sounds like a separate PR to me.

atumanov · 2017-03-01T05:15:37Z

-  ObjectInfo notification = {.obj_id = object_id, .is_deletion = true};
+  ObjectInfo notification;
+  notification.obj_id = object_id;
+  notification.is_deletion = true;


c++ struct init : ObjectInfo notification = {object_id, true};
but ok as is as well.

One problem with that kind of initialization (which I think I've run into in the past), is that if you change the struct definition it may still compile, but the values might correspond to different fields in the struct now.

atumanov · 2017-03-01T05:16:26Z

+  local_buf.object_id = oid;
+  local_buf.data_size = data_size;
+  local_buf.metadata_size = metadata_size;
+  local_buf.data = (uint8_t *) malloc(data_size);


might be best to use cpp struct initialization style here (with so many fields).

Looks like there may be some uninitialized fields here. This could be the cause of the Travis valgrind failure.

AmplabJenkins · 2017-03-01T05:17:57Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/150/
Test FAILed.

AmplabJenkins · 2017-03-01T08:13:15Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-03-01T08:13:15Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/152/
Test FAILed.

pcmoritz changed the title ~~Make Ray code C++ compatible~~ [WIP] Make Ray code C++ compatible Feb 26, 2017

pcmoritz force-pushed the cpp branch 2 times, most recently from c43d756 to 90c938b Compare February 28, 2017 19:28

pcmoritz force-pushed the cpp branch from 90c938b to 6c90d20 Compare February 28, 2017 22:46

pcmoritz force-pushed the cpp branch from 6c90d20 to 2b8f77c Compare March 1, 2017 02:24

pcmoritz force-pushed the cpp branch from a3d6277 to c2f37d9 Compare March 1, 2017 03:27

pcmoritz force-pushed the cpp branch from c2f37d9 to 4468d69 Compare March 1, 2017 04:28

convert Ray to C++

8419269

pcmoritz force-pushed the cpp branch from 4468d69 to 8419269 Compare March 1, 2017 04:37

const correctness

0c0cee3

linting

8ca1c62

atumanov reviewed Mar 1, 2017

View reviewed changes

pcmoritz changed the title ~~[WIP] Make Ray code C++ compatible~~ Make Ray code C++ compatible Mar 1, 2017

Fix valgrind error.

f0406dc

atumanov merged commit 793a102 into ray-project:master Mar 1, 2017

atumanov deleted the cpp branch March 1, 2017 09:17

This was referenced Mar 28, 2017

Replace UT data structures with C++ STL. #404

Closed

Convert codebase from C to C++. #406

Closed

robertnishihara mentioned this pull request Jan 3, 2019

Actor dummy object garbage collection #3593

Merged

Conversation

pcmoritz commented Feb 26, 2017

Uh oh!

AmplabJenkins commented Feb 28, 2017

Uh oh!

AmplabJenkins commented Feb 28, 2017

Uh oh!

AmplabJenkins commented Mar 1, 2017

Uh oh!

AmplabJenkins commented Mar 1, 2017

Uh oh!

AmplabJenkins commented Mar 1, 2017

Uh oh!

AmplabJenkins commented Mar 1, 2017

Uh oh!

robertnishihara commented Mar 1, 2017

Uh oh!

AmplabJenkins commented Mar 1, 2017

Uh oh!

AmplabJenkins commented Mar 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Mar 1, 2017

Uh oh!

AmplabJenkins commented Mar 1, 2017

Uh oh!

AmplabJenkins commented Mar 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants