Fix issue where the application would not terminate correctly when done. #19
Conversation
# Conflicts: # src/highlevel.cpp
There was a problem hiding this comment.
The changes look good, but I am unable to compile on Linux now. It fails with this error:
/home/mrodem/tmp/pc-nrfjprog-js/src/platform/linux/osfiles.cpp:37:10: fatal error:
'../osfiles.h' file not found
#include "../osfiles.h"
I suspect it has something to do with the changes that were introduced in 0f4c00c.
src/highlevel.cpp
Outdated
| keepDeviceOpen = false; | ||
| } | ||
|
|
||
| HighLevel::~HighLevel() |
There was a problem hiding this comment.
Necessary to keep empty destructor?
src/highlevel.cpp
Outdated
| HighLevel::~HighLevel() | ||
| {} | ||
|
|
||
| void HighLevel::CallFunction(Nan::NAN_METHOD_ARGS_TYPE info, parse_parameters_function_t parse, execute_function_t execute, return_function_t ret, const bool hasSerialNumber) |
There was a problem hiding this comment.
Would be good to stick to max-100-char-line-length, it makes it easier to read&review the code.
| return; | ||
| } | ||
|
|
||
| log("===============================================\n"); |
There was a problem hiding this comment.
Is this debug logging, that is, should it be removed?
There was a problem hiding this comment.
This is logs that are sent up on an error. So they are nice to have. They are mainly used in debugging, but make sense to send up in all cases.
src/highlevel.cpp
Outdated
| } | ||
| } | ||
|
|
||
| nrfjprogdll_err_t excuteError = baton->executeFunction(baton, probe); |
|
|
||
| if (progressEvent != nullptr) | ||
| { | ||
| auto handle = reinterpret_cast<uv_handle_t *>(progressEvent); |
There was a problem hiding this comment.
Why is a cast needed here? It's already of type uv_handle_t?
There was a problem hiding this comment.
It is a uv_async_t, so a cast is needed.
src/highlevel.cpp
Outdated
|
|
||
| uv_close(handle, [](uv_handle_t *handle) | ||
| { | ||
| free(handle); |
There was a problem hiding this comment.
progressEvent was created with new() at the top of the function, shouldn't it then be cleaned up with delete instead of free()?
src/highlevel.cpp
Outdated
| Nan::HandleScope scope; | ||
|
|
||
| auto baton = static_cast<Baton *>(req->data); | ||
| //TODO: Create an arrary of correct size instead of a way to large one. |
There was a problem hiding this comment.
semantics: 'too' instead of 'to'
or even better, implement what the TODO says :)
|
Still not able to compile on Linux unfortunately: |
Move creation and destruction of progress events to the execute function so that the progress event is not holding up the execution after application is done.