Do not push 'read' operation on Video constructor #120
Conversation
include/vcl/Video.h
Outdated
| @@ -523,6 +524,14 @@ namespace VCL { | |||
| */ | |||
| void read(const std::string &video_id ); | |||
There was a problem hiding this comment.
can we remove this function in another commit?
src/vcl/Video.cc
Outdated
| if (!_video_id.empty() && !is_read()) | ||
| _operations.push_front(std::make_shared<Read>()); | ||
| } | ||
| for (auto op : _operations) { |
There was a problem hiding this comment.
will auto& also work here?
There was a problem hiding this comment.
Yes, that should work fine.
There was a problem hiding this comment.
Last patch iterates through the elements that way.
src/vcl/Video.cc
Outdated
| bool is_empty = _operations.empty(); | ||
| auto first_op = !is_empty ? _operations.front() : NULL; | ||
| if (is_empty || (first_op && first_op->get_type() != READ)) { | ||
| if (!_video_id.empty() && !is_read()) |
There was a problem hiding this comment.
if !_video_id.empty(), we should throw
There was a problem hiding this comment.
That's the case where the execution reaches this function through the default constructor, which I believe should never happen (hence 'throw').
There was a problem hiding this comment.
I suppose that condition would be if (video_id.empty()) { throw ... } though.
src/vcl/Video.cc
Outdated
| std::shared_ptr<Operation> op = _operations[x]; | ||
| bool is_empty = _operations.empty(); | ||
| auto first_op = !is_empty ? _operations.front() : NULL; | ||
| if (is_empty || (first_op && first_op->get_type() != READ)) { |
There was a problem hiding this comment.
this can be simplified as (it is safe as .empty is evaluated first):
if ( (_operations.empty() || _operations.front()->get_type() != READ) && !is_read()) ) {
if (!_video_id.empty())
throw
_operations.push_front(std::make_shared());
There was a problem hiding this comment.
Yes, looks way more clean.
mahircg
force-pushed
the
remove-forced-read
branch
from
3185203 to
5c6bad9
Compare
|
I'll separate the UT and will add the patch to remove the 'read'. |
include/vcl/Video.h
Outdated
| * Stores a Read Operation in the list of operations | ||
| * to perform | ||
| * Checks whether the video pointed by the current video_id has | ||
| * has already been read. |
Currently, in some of the constructors of Video class, a 'read' operation is enqueued into the operation queue to ensure that the video is read before any other operation is performed. This, however, forces a read operation on on a video object even if there are no operations to apply (i.e. _operations is empty). This patch defers read operations until they are absolutely necessary. This is done by storing the operations in a linked list (rather than a vector), and adding a read operation to the head of the queue before perform_operations() invokes each operation in the queue. Signed-off-by: mahircg <[email protected]>
read() method is in protected scope, and cannot be called from any of the clients. Since read() function calls were removed in a prior commit, there's no need to keep the function. Signed-off-by: mahircg <[email protected]>
mahircg
force-pushed
the
remove-forced-read
branch
from
65100c9 to
a490996
Compare
Currently, in some of the constructors of Video class, a 'read' operation is
enqueued into the operation queue to ensure that the video is read before
any other operation is performed. This, however, forces a read operation on
on a video object even if there are no operations to apply (i.e.
_operations is empty).
This patch defers read operations until they are absolutely necessary.
This is done by storing the operations in a linked list (rather than a
vector), and adding a read operation to the head of the queue before
perform_operations() invokes each operation in the queue.
Signed-off-by: mahircg [email protected]