This is an archive of the discontinued LLVM Phabricator instance.

PosixPipes should not be copyable but should be movable.
ClosedPublic

Authored by chaoren on Apr 30 2015, 11:06 AM.

Diff Detail

Event Timeline

chaoren updated this revision to Diff 24751.Apr 30 2015, 11:06 AM
chaoren retitled this revision from to PosixPipes should not be copyable but should be movable..
chaoren updated this object.
chaoren edited the test plan for this revision. (Show Details)
chaoren added reviewers: ovyalov, clayborg.
chaoren added a subscriber: Unknown Object (MLST).
ovyalov added inline comments.Apr 30 2015, 12:16 PM
include/lldb/Host/posix/PipePosix.h
33

We may need to extend PipeBase with moving constructor/assignment operator to make moving work for Pipe instances.

source/Host/posix/PipePosix.cpp
132–135

Nit: - you may use PipePosix(PipePosix::kInvalidDescriptor, PipePosix::kInvalidDescriptor)

chaoren added inline comments.Apr 30 2015, 12:57 PM
include/lldb/Host/posix/PipePosix.h
33

I'm probably not understanding this correctly. We can't have virtual constructors. I don't think a virtual assignment operator makes much sense either. Do you want a = default move consturctor/assignment operator for PipeBase? (Which does't make sense either, since there's nothing to move in PipeBase.)

source/Host/posix/PipePosix.cpp
132–135

Seems unnecessary. It's neither more concise, nor more efficient. I think I prefer the more straight forward initialization.

ovyalov added inline comments.Apr 30 2015, 1:33 PM
include/lldb/Host/posix/PipePosix.h
33

My understanding that moving doesn't work properly for derived classes unless all base types define move constructor and/or assignment operator.
I think there is no harm to declare move constructor/assignment operator for PipeBase as default even if it doesn't have member fields - if PipeBase will be extended with member fields, no further move changes will be needed.

chaoren updated this revision to Diff 24769.Apr 30 2015, 2:13 PM

Future proof in case PipeBase ever gets extended with members fields.

clayborg accepted this revision.Apr 30 2015, 2:15 PM
clayborg edited edge metadata.

Looks fine

This revision is now accepted and ready to land.Apr 30 2015, 2:15 PM
ovyalov accepted this revision.Apr 30 2015, 9:53 PM
ovyalov edited edge metadata.

lgtm

This revision was automatically updated to reflect the committed changes.