Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

build: fix or suppress warnings #8485

Closed
wants to merge 4 commits into from
Closed

build: fix or suppress warnings #8485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7f4a50b
src: suppress orphan warnings
refack Oct 8, 2014
f91e496
src: fix C4800 implicit casting of int to bool
refack Oct 8, 2014
97ca146
src: fix remaining warnings
refack Oct 8, 2014
f4f2b6d
deps: suppress known warnings
refack Sep 28, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,7 @@ deps/zlib/zlib.target.mk
# test artifacts
tools/faketime
icu_config.gypi


#Jetbrains project files (WebStorm / Clion)
/.idea/
13 changes: 11 additions & 2 deletions common.gypi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,13 @@
'BufferSecurityCheck': 'true',
'ExceptionHandling': 1, # /EHsc
'SuppressStartupBanner': 'true',
'WarnAsError': 'false',
'WarnAsError': 'true',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whyyyy???

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it works!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please turn it off, I have already explained why it is a bad thing for node.

},
# 4221 - linker warning about object not exporting new symbols
'VCLibrarianTool': {
'AdditionalOptions': [
'/ignore:4221',
],
},
'VCLinkerTool': {
'conditions': [
Expand All @@ -151,7 +155,12 @@
],
},
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is true unconditinaly that if MSVS is used then it should not use cygwin

'msvs_disabled_warnings': [4351, 4355, 4800],
# 4351, 4355, 4800 - Legacy
# 4244 - when passing an int64 as int, and truncation will happen
# 4267 - int64 passed as int, truncation might happen (depends on linkage)
# 4530 - No exception semantics (leaking from MS STL xlocale)
# 4996 - winsock ip4 calls deprecated
'msvs_disabled_warnings': [4351, 4355, 4800, 4244, 4267, 4530, 4996],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the exception of maybe C4351, I don't think it's a good idea to disable these warnings. And for warnings that cannot be fixed, like the ones about deprecation, it would be better to disable them in the source file using #pragma

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are here for deps sake. Only 4244, 4267 are allowed in node code.

'conditions': [
['OS == "win"', {
'msvs_cygwin_shell': 0, # prevent actions from trying to use cygwin
Expand Down
8 changes: 8 additions & 0 deletions node.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,10 @@
'NODE_TAG="<(node_tag)"',
'NODE_V8_OPTIONS="<(node_v8_options)"',
],
# these should not occur in `node` code. Only allowed:
# 4244 - when passing an int64 as int, and truncation will happen
# 4267 - int64 passed as int, truncation might happen (depends on linkage)
'msvs_disabled_warnings!': [4351, 4355, 4800, 4530, 4996],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing them or hiding them per file is too noisy from my POV:
https://gist.github.com/refack/65eb7b126e1c9b697134


'conditions': [
[ 'v8_enable_i18n_support==1', {
Expand Down Expand Up @@ -322,6 +326,10 @@
[ 'OS=="win"', {
'sources': [
'src/res/node.rc',
],
# we need to use node's preferred "win32" rather than gyp's preferred "win"
'defines!': [
'PLATFORM="win"',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some comment with explanation here.

],
'defines': [
'FD_SETSIZE=1024',
Expand Down
12 changes: 6 additions & 6 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2531,7 +2531,7 @@ void NeedImmediateCallbackGetter(Local<String> property,
Environment* env = Environment::GetCurrent(info.GetIsolate());
const uv_check_t* immediate_check_handle = env->immediate_check_handle();
bool active = uv_is_active(
reinterpret_cast<const uv_handle_t*>(immediate_check_handle));
reinterpret_cast<const uv_handle_t*>(immediate_check_handle)) != 0;
info.GetReturnValue().Set(active);
}

Expand All @@ -2545,7 +2545,7 @@ static void NeedImmediateCallbackSetter(

uv_check_t* immediate_check_handle = env->immediate_check_handle();
bool active = uv_is_active(
reinterpret_cast<const uv_handle_t*>(immediate_check_handle));
reinterpret_cast<const uv_handle_t*>(immediate_check_handle)) != 0;

if (active == value->BooleanValue())
return;
Expand Down Expand Up @@ -3749,19 +3749,19 @@ int Start(int argc, char** argv) {
if (use_debug_agent)
EnableDebug(env);

bool more;
int more;
do {
more = uv_run(env->event_loop(), UV_RUN_ONCE);
if (more == false) {
if (more == 0) {
EmitBeforeExit(env);

// Emit `beforeExit` if the loop became alive either after emitting
// event, or after running some callbacks.
more = uv_loop_alive(env->event_loop());
if (uv_run(env->event_loop(), UV_RUN_NOWAIT) != 0)
more = true;
more = 1;
}
} while (more == true);
} while (more != 0);
code = EmitExit(env);
RunAtExit(env);

Expand Down
13 changes: 5 additions & 8 deletions src/node_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1128,7 +1128,7 @@ void SSLWrap<Base>::OnClientHello(void* arg,
hello_obj->Set(env->tls_ticket_string(),
Boolean::New(env->isolate(), hello.has_ticket()));
hello_obj->Set(env->ocsp_request_string(),
Boolean::New(env->isolate(), hello.ocsp_request()));
Boolean::New(env->isolate(), hello.ocsp_request() != 0));

Local<Value> argv[] = { hello_obj };
w->MakeCallback(env->onclienthello_string(), ARRAY_SIZE(argv), argv);
Expand Down Expand Up @@ -1495,7 +1495,7 @@ template <class Base>
void SSLWrap<Base>::IsSessionReused(const FunctionCallbackInfo<Value>& args) {
HandleScope scope(args.GetIsolate());
Base* w = Unwrap<Base>(args.Holder());
bool yes = SSL_session_reused(w->ssl_);
bool yes = SSL_session_reused(w->ssl_) != 0;
args.GetReturnValue().Set(yes);
}

Expand Down Expand Up @@ -2779,11 +2779,8 @@ bool CipherBase::Update(const char* data,

*out_len = len + EVP_CIPHER_CTX_block_size(&ctx_);
*out = new unsigned char[*out_len];
return EVP_CipherUpdate(&ctx_,
*out,
out_len,
reinterpret_cast<const unsigned char*>(data),
len);
const unsigned char* cdata = reinterpret_cast<const unsigned char*>(data);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

80 column limit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes.

return EVP_CipherUpdate(&ctx_, *out, out_len, cdata, len) != 0;
}


Expand Down Expand Up @@ -2838,7 +2835,7 @@ void CipherBase::Update(const FunctionCallbackInfo<Value>& args) {
bool CipherBase::SetAutoPadding(bool auto_padding) {
if (!initialised_)
return false;
return EVP_CIPHER_CTX_set_padding(&ctx_, auto_padding);
return EVP_CIPHER_CTX_set_padding(&ctx_, auto_padding) != 0;
}


Expand Down
5 changes: 5 additions & 0 deletions src/node_file.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@ using v8::Value;

#define THROW_BAD_ARGS TYPE_ERROR("Bad argument")

#ifdef _MSC_VER
// no matching operator delete found;
// memory will not be freed if initialization throws an exception
#pragma warning(disable: 4291)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which line does it happen on?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a declaration type warning. The compiler refers to whole class declared from line 74

#endif
class FSReqWrap: public ReqWrap<uv_fs_t> {
public:
void* operator new(size_t size) { return new char[size]; }
Expand Down
7 changes: 6 additions & 1 deletion src/node_os.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,11 @@ static void GetOSRelease(const FunctionCallbackInfo<Value>& args) {
OSVERSIONINFO info;

info.dwOSVersionInfoSize = sizeof(info);
#ifdef _MSC_VER
// `GetVersionEx` has been deprecated. It seems that it will only report the
// version this code is build for (so it can be mocked during build)
#pragma warning(disable: 4996)
#endif
if (GetVersionEx(&info) == 0)
return;

Expand Down Expand Up @@ -288,7 +293,7 @@ static void GetInterfaceAddresses(const FunctionCallbackInfo<Value>& args) {
Integer::NewFromUnsigned(env->isolate(), scopeid));
}

const bool internal = interfaces[i].is_internal;
const bool internal = interfaces[i].is_internal != 0;
o->Set(env->internal_string(),
internal ? True(env->isolate()) : False(env->isolate()));

Expand Down
4 changes: 2 additions & 2 deletions src/node_win32_etw_provider.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,8 @@ INLINE bool NODE_NET_SOCKET_READ_ENABLED();
INLINE bool NODE_NET_SOCKET_WRITE_ENABLED();
INLINE bool NODE_V8SYMBOL_ENABLED();

#define NODE_NET_SOCKET_READ(arg0, arg1)
#define NODE_NET_SOCKET_WRITE(arg0, arg1)
#define NODE_NET_SOCKET_READ(arg0, arg1, arg2, arg3, arg4)
#define NODE_NET_SOCKET_WRITE(arg0, arg1, arg2, arg3, arg4)
} // namespace node

#endif // SRC_NODE_WIN32_ETW_PROVIDER_H_
2 changes: 1 addition & 1 deletion src/string_bytes.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ size_t base64_decode(char* buf,
const TypeName* srcEnd = src + srcLen;

while (src < srcEnd && dst < dstEnd) {
int remaining = srcEnd - src;
size_t remaining = srcEnd - src;

while (unbase64(*src) < 0 && src < srcEnd)
src++, remaining--;
Expand Down