Conversation
Fixes for compiler warnings. The majority were mismatched types and some uninitialized variables. Added flags to suppress some deprecation/security warnings. Remove commented out line More compiler warning fixes Added some Intellisense warnings.
mhoyer-streamlabs
force-pushed
the
fix-compiler-warnings
branch
from
b7b32c1 to
d7b3fd5
Compare
| Napi::HandleScope scope(env); | ||
| int length = info.Length(); | ||
| size_t length = info.Length(); | ||
| this->uid = 0; |
There was a problem hiding this comment.
Just curious why not initialize the variable in the constructor like so:
osn::AudioTrack::AudioTrack(const Napi::CallbackInfo &info) : Napi::ObjectWrap<osn::AudioTrack>(info), uid(0)
There was a problem hiding this comment.
Old habits die hard...just fell back on how I used to do it.
| Napi::HandleScope scope(env); | ||
| int length = info.Length(); | ||
| size_t length = info.Length(); | ||
| this->uid = 0; |
There was a problem hiding this comment.
I would initialize the variable in the constructor like so (unless there is some reason not to?):
osn::AudioEncoder::AudioEncoder(const Napi::CallbackInfo &info) : Napi::ObjectWrap<osn::AudioEncoder>(info), uid(0)
| Napi::HandleScope scope(env); | ||
| int length = info.Length(); | ||
| size_t length = info.Length(); | ||
| this->uid = 0; |
There was a problem hiding this comment.
I leave this up to you I would init this one in the constructor.
| Napi::HandleScope scope(env); | ||
| int length = info.Length(); | ||
| size_t length = info.Length(); | ||
| this->moduleId = 0; |
There was a problem hiding this comment.
I was thinking best to define in constructor?
| #pragma warning(push, 0) | ||
| #include <node.h> | ||
| #pragma warning(pop) | ||
| //#include <node.h> |
There was a problem hiding this comment.
do we want to remove this or does the old deprecated code have value?
There was a problem hiding this comment.
That actually wasn't intentional, just forgot to go back and delete.
| Napi::HandleScope scope(env); | ||
| int length = info.Length(); | ||
| size_t length = info.Length(); | ||
| this->uid = 0; |
There was a problem hiding this comment.
define in constructor?
| #pragma warning(push, 0) | ||
| #include <node.h> | ||
| #pragma warning(pop) | ||
| //#include <node.h> |
There was a problem hiding this comment.
did we want to keep the dead code?
| Napi::HandleScope scope(env); | ||
| int length = info.Length(); | ||
| size_t length = info.Length(); | ||
| this->itemId = 0; |
There was a problem hiding this comment.
define in constructor?
| Napi::HandleScope scope(env); | ||
| int length = info.Length(); | ||
| size_t length = info.Length(); | ||
| this->uid = 0; |
There was a problem hiding this comment.
define in constructor?
|
|
||
| for (size_t j = 0; j < item->magnitude.size(); j++) { | ||
| magnitude.Set(j, Napi::Number::New(env, item->magnitude[j])); | ||
| magnitude.Set(static_cast<uint32_t>(j), Napi::Number::New(env, item->magnitude[j])); |
There was a problem hiding this comment.
We could have made the loop variable a uint32_t to avoid the static_cast but I'm not sure how big of an improvement it would be since the uint32_t will just wrap around if we were to go past UINT32_MAX which can be quite a pain. But I'm assuming this code never goes past that limit.
Example:
for (uint32_t j = 0; j < item->magnitude.size(); j++) {
There was a problem hiding this comment.
Fair point, I just left the size_t as that's what .size() returns and used the cast on the line with the warning.
sandboxcoder
left a comment
sandboxcoder
left a comment
There was a problem hiding this comment.
I left some comments but I think they are all trivial (?) thanks this looks good. Big improvement
3 participants
Description
Code changes fix compiler warnings, including both build warnings and Intellisense warnings. The majority of changes involve types and adding explicit casting. Other changes include initializing variables, checking for null pointers, and adding flags to ignore deprecation and security warnings. A handful of deprecated function calls were updated.
Motivation and Context
These changes were made so that legitimate warnings do not get lost in a sea of inconsequential warnings.
How Has This Been Tested?
Yarn tests completed successfully.
Types of changes
Checklist: