-
Notifications
You must be signed in to change notification settings - Fork 101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix compiler warnings #1507
base: staging
Are you sure you want to change the base?
Fix compiler warnings #1507
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.
b7b32c1
to
d7b3fd5
Compare
@@ -46,7 +46,8 @@ osn::AudioTrack::AudioTrack(const Napi::CallbackInfo &info) : Napi::ObjectWrap<o | |||
{ | |||
Napi::Env env = info.Env(); | |||
Napi::HandleScope scope(env); | |||
int length = info.Length(); | |||
size_t length = info.Length(); | |||
this->uid = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old habits die hard...just fell back on how I used to do it.
@@ -43,7 +43,8 @@ osn::AudioEncoder::AudioEncoder(const Napi::CallbackInfo &info) : Napi::ObjectWr | |||
{ | |||
Napi::Env env = info.Env(); | |||
Napi::HandleScope scope(env); | |||
int length = info.Length(); | |||
size_t length = info.Length(); | |||
this->uid = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
@@ -44,7 +44,8 @@ osn::Delay::Delay(const Napi::CallbackInfo &info) : Napi::ObjectWrap<osn::Delay> | |||
{ | |||
Napi::Env env = info.Env(); | |||
Napi::HandleScope scope(env); | |||
int length = info.Length(); | |||
size_t length = info.Length(); | |||
this->uid = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I leave this up to you I would init this one in the constructor.
@@ -56,7 +56,8 @@ osn::Module::Module(const Napi::CallbackInfo &info) : Napi::ObjectWrap<osn::Modu | |||
{ | |||
Napi::Env env = info.Env(); | |||
Napi::HandleScope scope(env); | |||
int length = info.Length(); | |||
size_t length = info.Length(); | |||
this->moduleId = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking best to define in constructor?
#include <node.h> | ||
#pragma warning(pop) | ||
//#include <node.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to remove this or does the old deprecated code have value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That actually wasn't intentional, just forgot to go back and delete.
@@ -44,7 +44,8 @@ osn::Reconnect::Reconnect(const Napi::CallbackInfo &info) : Napi::ObjectWrap<osn | |||
{ | |||
Napi::Env env = info.Env(); | |||
Napi::HandleScope scope(env); | |||
int length = info.Length(); | |||
size_t length = info.Length(); | |||
this->uid = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
define in constructor?
#include <node.h> | ||
#pragma warning(pop) | ||
//#include <node.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did we want to keep the dead code?
@@ -78,7 +78,8 @@ osn::SceneItem::SceneItem(const Napi::CallbackInfo &info) : Napi::ObjectWrap<osn | |||
{ | |||
Napi::Env env = info.Env(); | |||
Napi::HandleScope scope(env); | |||
int length = info.Length(); | |||
size_t length = info.Length(); | |||
this->itemId = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
define in constructor?
@@ -52,7 +52,8 @@ osn::VideoEncoder::VideoEncoder(const Napi::CallbackInfo &info) : Napi::ObjectWr | |||
{ | |||
Napi::Env env = info.Env(); | |||
Napi::HandleScope scope(env); | |||
int length = info.Length(); | |||
size_t length = info.Length(); | |||
this->uid = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
define in constructor?
@@ -135,21 +137,21 @@ void globalCallback::worker() | |||
Napi::Array input_peak = Napi::Array::New(env); | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, I just left the size_t as that's what .size() returns and used the cast on the line with the warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments but I think they are all trivial
(?) thanks this looks good. Big improvement
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: