Updated DeviceDefender task interface changes #305
Conversation
| } | ||
|
|
||
| ReportTask::ReportTask(ReportTask &&toMove) noexcept | ||
| : OnTaskCancelled(std::move(toMove.OnTaskCancelled)), cancellationUserdata(toMove.cancellationUserdata), | ||
| m_allocator(toMove.m_allocator), m_status(toMove.m_status), m_taskConfig(std::move(toMove.m_taskConfig)), | ||
| m_owningTask(toMove.m_owningTask), m_lastError(toMove.m_lastError) | ||
| m_owningTask(toMove.m_owningTask), m_lastError(toMove.m_lastError), |
There was a problem hiding this comment.
Is this constructor necessary?
There was a problem hiding this comment.
I'm not sure. It was already there so I made adjustments as necessary rather than figure out if it was safe to delete.
| toMove.m_allocator = nullptr; | ||
| toMove.m_taskConfig = m_taskConfig; | ||
| toMove.m_owningTask = m_owningTask; | ||
| m_owningTask = nullptr; |
There was a problem hiding this comment.
This doesn't look right to me. Why are we pushing things back into the thing we're moving out of?
For example here are several side-affect sequences, neither of which seem correct:
m_taskConfig(std::move(toMove.m_taskConfig))
toMove.m_taskConfig = m_taskConfig;
m_owningTask(toMove.m_owningTask) // copy
toMove.m_owningTask = m_owningTask // does nothing
m_owningTask = nullptr; // why?
toMove.m_owningTask = nullptr;
There was a problem hiding this comment.
Deleted this entire function
| m_lastError = | ||
| aws_iotdevice_defender_config_create(&m_taskConfig, allocator, &thingNameCursor, reportFormat); | ||
| if (AWS_OP_SUCCESS == m_lastError) |
There was a problem hiding this comment.
trivial: it's a little weird to save the AWS_OP_ result in the same variable that's supposed to hold an AWS_ERROR_. They're conceptually two different types, even if they both happen to be ints
consider either
if (aws_iotdevice_defender_config_create(...) == AWS_OP_SUCCESS)
or
int result = aws_iotdevice_defender_config_create(...);
if (result == AWS_OP_SUCESS)
...
| } | ||
|
|
||
| ReportTask::ReportTask(ReportTask &&toMove) noexcept | ||
| : OnTaskCancelled(std::move(toMove.OnTaskCancelled)), cancellationUserdata(toMove.cancellationUserdata), | ||
| m_allocator(toMove.m_allocator), m_status(toMove.m_status), m_taskConfig(std::move(toMove.m_taskConfig)), | ||
| m_owningTask(toMove.m_owningTask), m_lastError(toMove.m_lastError) | ||
| m_owningTask(toMove.m_owningTask), m_lastError(toMove.m_lastError), |
There was a problem hiding this comment.
Are you sure you want to support move operations? It's a lot of subtle stuff that can accidentally go wrong. And if we ever need to pass a pointer to the C++ class down into C as user_data then then move is illegal
There was a problem hiding this comment.
I didn't, but after evaluating that callback_userdata is given a value of this, it's clear that a move after that won't be valid. This class has to be non-copyable, non-moveable. Making the adjustments
| &m_owningTask, | ||
| this->m_taskConfig, | ||
| m_mqttConnection->GetUnderlyingConnection(), | ||
| aws_event_loop_group_get_next_loop(m_eventLoopGroup.GetUnderlyingHandle()))) | ||
| { | ||
| this->m_lastError = aws_last_error(); | ||
| aws_raise_error(this->m_lastError); |
There was a problem hiding this comment.
trivial: there's no point in re-raising the error
| } | ||
| } | ||
| return AWS_OP_SUCCESS; | ||
| return return_code; |
There was a problem hiding this comment.
With this change, if the first if statement fails, we'll end up returning AWS_OP_ERR without having ever raised an error
There was a problem hiding this comment.
True. Raising AWS_ERROR_INVALID_STATE in this situation
…error logic to raise proper aws error and added test
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.