Skip to content

Tighten up warnings#315

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.

Sign up for GitHub

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

Closed
mcheshkov wants to merge 4 commits into from
Closed

Tighten up warnings #315

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
abb68b9
test: Tighten up compiler warnings
mcheshkov Aug 14, 2018
c4c2290
Use separate macro for void returning throw
mcheshkov Aug 14, 2018
3d74ec9
Comment unused parameter names
mcheshkov Aug 14, 2018
5c9c90b
Remove excessive semicolon
mcheshkov Aug 14, 2018
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
81 changes: 46 additions & 35 deletions napi-inl.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -27,6 +27,9 @@ namespace details {
#define NAPI_THROW_IF_FAILED(env, status, ...) \
if ((status) != napi_ok) throw Error::New(env);

#define NAPI_THROW_IF_FAILED_VOID(env, status) \
if ((status) != napi_ok) throw Error::New(env);

#else // NAPI_CPP_EXCEPTIONS

#define NAPI_THROW(e) (e).ThrowAsJavaScriptException();
Expand All@@ -40,6 +43,14 @@ namespace details {
return __VA_ARGS__; \
}

// Usually ony could use NAPI_THROW_IF_FAILED(env, status, void()),

@gabrielschulhofgabrielschulhofSep 7, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: How about we re-phrase this to

// We need a _VOID version of this macro to avoid warnings resulting from// leaving the NAPI_THROW_IF_FAILED `...` argument empty.

// but it is forbidden to return value from cons, and forbidden to leave "..." argument empty
#define NAPI_THROW_IF_FAILED_VOID(env, status) \
if ((status) != napi_ok) { \
Error::New(env).ThrowAsJavaScriptException(); \
return; \
}

#endif // NAPI_CPP_EXCEPTIONS

#define NAPI_FATAL_IF_FAILED(status, location, message) \
Expand DownExpand Up@@ -160,7 +171,7 @@ struct AccessorCallbackData {
napi_value exports) { \
return Napi::RegisterModule(env, exports, regfunc); \
} \
NAPI_MODULE(modname, __napi_ ## regfunc);
NAPI_MODULE(modname, __napi_ ## regfunc)

// Adapt the NAPI_MODULE registration function:
// - Wrap the arguments in NAPI wrappers.
Expand DownExpand Up@@ -867,21 +878,21 @@ template <typename ValueType>
inline void Object::Set(napi_value key, const ValueType& value) {
napi_status status =
napi_set_property(_env, _value, key, Value::From(_env, value));
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
}

template <typename ValueType>
inline void Object::Set(Value key, const ValueType& value) {
napi_status status =
napi_set_property(_env, _value, key, Value::From(_env, value));
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
}

template <typename ValueType>
inline void Object::Set(const char* utf8name, const ValueType& value) {
napi_status status =
napi_set_named_property(_env, _value, utf8name, Value::From(_env, value));
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
}

template <typename ValueType>
Expand DownExpand Up@@ -929,7 +940,7 @@ template <typename ValueType>
inline void Object::Set(uint32_t index, const ValueType& value) {
napi_status status =
napi_set_element(_env, _value, index, Value::From(_env, value));
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
}

inline bool Object::Delete(uint32_t index) {
Expand All@@ -949,19 +960,19 @@ inline Array Object::GetPropertyNames() {
inline void Object::DefineProperty(const PropertyDescriptor& property) {
napi_status status = napi_define_properties(_env, _value, 1,
reinterpret_cast<const napi_property_descriptor*>(&property));
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
}

inline void Object::DefineProperties(const std::initializer_list<PropertyDescriptor>& properties) {
napi_status status = napi_define_properties(_env, _value, properties.size(),
reinterpret_cast<const napi_property_descriptor*>(properties.begin()));
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
}

inline void Object::DefineProperties(const std::vector<PropertyDescriptor>& properties) {
napi_status status = napi_define_properties(_env, _value, properties.size(),
reinterpret_cast<const napi_property_descriptor*>(properties.data()));
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
}

inline bool Object::InstanceOf(const Function& constructor) const {
Expand DownExpand Up@@ -1171,7 +1182,7 @@ inline void ArrayBuffer::EnsureInfo() const {
// since they can never change during the lifetime of the ArrayBuffer.
if (_data == nullptr) {
napi_status status = napi_get_arraybuffer_info(_env, _value, &_data, &_length);
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
}
}

Expand DownExpand Up@@ -1222,7 +1233,7 @@ inline DataView::DataView(napi_env env, napi_value value) : Object(env, value) {
&_data /* data */,
nullptr /* arrayBuffer */,
nullptr /* byteOffset */);
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
}

inline Napi::ArrayBuffer DataView::ArrayBuffer() const {
Expand DownExpand Up@@ -1466,7 +1477,7 @@ inline TypedArrayOf<T>::TypedArrayOf(napi_env env, napi_value value)
: TypedArray(env, value), _data(nullptr) {
napi_status status = napi_get_typedarray_info(
_env, _value, &_type, &_length, reinterpret_cast<void**>(&_data), nullptr, nullptr);
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
}

template <typename T>
Expand DownExpand Up@@ -1615,7 +1626,7 @@ inline Promise::Deferred Promise::Deferred::New(napi_env env) {

inline Promise::Deferred::Deferred(napi_env env) : _env(env) {
napi_status status = napi_create_promise(_env, &_deferred, &_promise);
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
}

inline Promise Promise::Deferred::Promise() const {
Expand All@@ -1624,12 +1635,12 @@ inline Promise Promise::Deferred::Promise() const {

inline void Promise::Deferred::Resolve(napi_value value) const {
napi_status status = napi_resolve_deferred(_env, _deferred, value);
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
}

inline void Promise::Deferred::Reject(napi_value value) const {
napi_status status = napi_reject_deferred(_env, _deferred, value);
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
}

inline Promise::Promise(napi_env env, napi_value value) : Object(env, value) {
Expand DownExpand Up@@ -1748,7 +1759,7 @@ inline void Buffer<T>::EnsureInfo() const {
size_t byteLength;
void* voidData;
napi_status status = napi_get_buffer_info(_env, _value, &voidData, &byteLength);
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
_length = byteLength / sizeof (T);
_data = static_cast<T*>(voidData);
}
Expand DownExpand Up@@ -1884,7 +1895,7 @@ inline void Error::ThrowAsJavaScriptException() const {
HandleScope scope(_env);
if (!IsEmpty()) {
napi_status status = napi_throw(_env, Value());
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
}
}

Expand DownExpand Up@@ -2073,7 +2084,7 @@ template <typename T>
inline void Reference<T>::Reset() {
if (_ref != nullptr) {
napi_status status = napi_delete_reference(_env, _ref);
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
_ref = nullptr;
}
}
Expand All@@ -2086,7 +2097,7 @@ inline void Reference<T>::Reset(const T& value, uint32_t refcount) {
napi_value val = value;
if (val != nullptr) {
napi_status status = napi_create_reference(_env, value, refcount, &_ref);
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
}
}

Expand DownExpand Up@@ -2360,7 +2371,7 @@ inline CallbackInfo::CallbackInfo(napi_env env, napi_callback_info info)
_argc = _staticArgCount;
_argv = _staticArgs;
napi_status status = napi_get_cb_info(env, info, &_argc, _argv, &_this, &_data);
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);

if (_argc > _staticArgCount) {
// Use either a fixed-size array (on the stack) or a dynamically-allocated
Expand All@@ -2369,7 +2380,7 @@ inline CallbackInfo::CallbackInfo(napi_env env, napi_callback_info info)
_argv = _dynamicArgs;

status = napi_get_cb_info(env, info, &_argc, _argv, nullptr, nullptr);
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
}
}

Expand DownExpand Up@@ -2426,7 +2437,7 @@ inline PropertyDescriptor
PropertyDescriptor::Accessor(const char* utf8name,
Getter getter,
napi_property_attributes attributes,
void* data) {
void* /*data*/) {
typedef details::CallbackData<Getter, Napi::Value> CbData;
// TODO: Delete when the function is destroyed
auto callbackData = new CbData({ getter, nullptr });
Expand DownExpand Up@@ -2455,7 +2466,7 @@ template <typename Getter>
inline PropertyDescriptor PropertyDescriptor::Accessor(napi_value name,
Getter getter,
napi_property_attributes attributes,
void* data) {
void* /*data*/) {
typedef details::CallbackData<Getter, Napi::Value> CbData;
// TODO: Delete when the function is destroyed
auto callbackData = new CbData({ getter, nullptr });
Expand DownExpand Up@@ -2486,7 +2497,7 @@ inline PropertyDescriptor PropertyDescriptor::Accessor(const char* utf8name,
Getter getter,
Setter setter,
napi_property_attributes attributes,
void* data) {
void* /*data*/) {
typedef details::AccessorCallbackData<Getter, Setter> CbData;
// TODO: Delete when the function is destroyed
auto callbackData = new CbData({ getter, setter });
Expand DownExpand Up@@ -2517,7 +2528,7 @@ inline PropertyDescriptor PropertyDescriptor::Accessor(napi_value name,
Getter getter,
Setter setter,
napi_property_attributes attributes,
void* data) {
void* /*data*/) {
typedef details::AccessorCallbackData<Getter, Setter> CbData;
// TODO: Delete when the function is destroyed
auto callbackData = new CbData({ getter, setter });
Expand DownExpand Up@@ -2548,7 +2559,7 @@ template <typename Callable>
inline PropertyDescriptor PropertyDescriptor::Function(const char* utf8name,
Callable cb,
napi_property_attributes attributes,
void* data) {
void* /*data*/) {
typedef decltype(cb(CallbackInfo(nullptr, nullptr))) ReturnType;
typedef details::CallbackData<Callable, ReturnType> CbData;
// TODO: Delete when the function is destroyed
Expand DownExpand Up@@ -2578,7 +2589,7 @@ template <typename Callable>
inline PropertyDescriptor PropertyDescriptor::Function(napi_value name,
Callable cb,
napi_property_attributes attributes,
void* data) {
void* /*data*/) {
typedef decltype(cb(CallbackInfo(nullptr, nullptr))) ReturnType;
typedef details::CallbackData<Callable, ReturnType> CbData;
// TODO: Delete when the function is destroyed
Expand DownExpand Up@@ -2659,7 +2670,7 @@ inline ObjectWrap<T>::ObjectWrap(const Napi::CallbackInfo& callbackInfo) {
napi_ref ref;
T* instance = static_cast<T*>(this);
status = napi_wrap(env, wrapper, instance, FinalizeCallback, nullptr, &ref);
NAPI_THROW_IF_FAILED(env, status)
NAPI_THROW_IF_FAILED_VOID(env, status);

Reference<Object>* instanceRef = instance;
*instanceRef = Reference<Object>(env, ref);
Expand DownExpand Up@@ -3027,7 +3038,7 @@ inline HandleScope::HandleScope(napi_env env, napi_handle_scope scope)

inline HandleScope::HandleScope(Napi::Env env) : _env(env) {
napi_status status = napi_open_handle_scope(_env, &_scope);
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
}

inline HandleScope::~HandleScope() {
Expand All@@ -3052,7 +3063,7 @@ inline EscapableHandleScope::EscapableHandleScope(

inline EscapableHandleScope::EscapableHandleScope(Napi::Env env) : _env(env) {
napi_status status = napi_open_escapable_handle_scope(_env, &_scope);
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
}

inline EscapableHandleScope::~EscapableHandleScope() {
Expand DownExpand Up@@ -3120,11 +3131,11 @@ inline AsyncWorker::AsyncWorker(const Object& receiver,
napi_value resource_id;
napi_status status = napi_create_string_latin1(
_env, resource_name, NAPI_AUTO_LENGTH, &resource_id);
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);

status = napi_create_async_work(_env, resource, resource_id, OnExecute,
OnWorkComplete, this, &_work);
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
}

inline AsyncWorker::~AsyncWorker() {
Expand DownExpand Up@@ -3165,12 +3176,12 @@ inline Napi::Env AsyncWorker::Env() const {

inline void AsyncWorker::Queue() {
napi_status status = napi_queue_async_work(_env, _work);
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
}

inline void AsyncWorker::Cancel() {
napi_status status = napi_cancel_async_work(_env, _work);
NAPI_THROW_IF_FAILED(_env, status);
NAPI_THROW_IF_FAILED_VOID(_env, status);
}

inline ObjectReference& AsyncWorker::Receiver() {
Expand All@@ -3193,7 +3204,7 @@ inline void AsyncWorker::SetError(const std::string& error) {
_error = error;
}

inline void AsyncWorker::OnExecute(napi_env env, void* this_pointer) {
inline void AsyncWorker::OnExecute(napi_env /*env*/, void* this_pointer) {
AsyncWorker* self = static_cast<AsyncWorker*>(this_pointer);
#ifdef NAPI_CPP_EXCEPTIONS
try {
Expand All@@ -3207,7 +3218,7 @@ inline void AsyncWorker::OnExecute(napi_env env, void* this_pointer) {
}

inline void AsyncWorker::OnWorkComplete(
napi_env env, napi_status status, void* this_pointer) {
napi_env /*env*/, napi_status status, void* this_pointer) {

@mhdawsonmhdawsonSep 6, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is it necessary to comment out some of the parameter names like this?

@mcheshkovmcheshkovSep 6, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Parameter of a function definition, that is not used in function body generate warning with -Wunused-parameter enabled.

https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

Since library is header-only, even function definitions will be compiled with library user (as opposed to library author) flags. If library user has this warning enabled one will get warnings coming from library code, and to fix/hide them one will have to disable warnings for every compilation unit that includes this library, or wrap include with #pragma push-ignore-pop dance. Or fork/vendor library and fix warnings.

AsyncWorker* self = static_cast<AsyncWorker*>(this_pointer);
if (status != napi_cancelled) {
HandleScope scope(self->_env);
Expand Down
4 changes: 2 additions & 2 deletions test/arraybuffer.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -63,7 +63,7 @@ Value CreateExternalBufferWithFinalize(const CallbackInfo& info) {
info.Env(),
data,
testLength,
[](Env env, void* finalizeData) {
[](Env /*env*/, void* finalizeData) {
delete[] static_cast<uint8_t*>(finalizeData);
finalizeCount++;
});
Expand DownExpand Up@@ -92,7 +92,7 @@ Value CreateExternalBufferWithFinalizeHint(const CallbackInfo& info) {
info.Env(),
data,
testLength,
[](Env env, void* finalizeData, char* finalizeHint) {
[](Env /*env*/, void* finalizeData, char* /*finalizeHint*/) {
delete[] static_cast<uint8_t*>(finalizeData);
finalizeCount++;
},
Expand Down
2 changes: 2 additions & 0 deletions test/binding.gyp
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -28,6 +28,8 @@
],
'include_dirs': ["<!@(node -p \"require('../').include\")"],
'dependencies': ["<!(node -p \"require('../').gyp\")"],
'cflags': [ '-Werror', '-Wall', '-Wextra', '-Wpedantic', '-Wunused-parameter' ],
'cflags_cc': [ '-Werror', '-Wall', '-Wextra', '-Wpedantic', '-Wunused-parameter' ],
},
'targets': [
{
Expand Down
4 changes: 2 additions & 2 deletions test/buffer.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -68,7 +68,7 @@ Value CreateExternalBufferWithFinalize(const CallbackInfo& info) {
info.Env(),
data,
testLength,
[](Env env, uint16_t* finalizeData) {
[](Env /*env*/, uint16_t* finalizeData) {
delete[] finalizeData;
finalizeCount++;
});
Expand DownExpand Up@@ -97,7 +97,7 @@ Value CreateExternalBufferWithFinalizeHint(const CallbackInfo& info) {
info.Env(),
data,
testLength,
[](Env env, uint16_t* finalizeData, char* finalizeHint) {
[](Env /*env*/, uint16_t* finalizeData, char* /*finalizeHint*/) {
delete[] finalizeData;
finalizeCount++;
},
Expand Down
2 changes: 1 addition & 1 deletion test/error.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -154,7 +154,7 @@ void CatchAndRethrowErrorThatEscapesScope(const CallbackInfo& info) {

#endif // NAPI_CPP_EXCEPTIONS

void ThrowFatalError(const CallbackInfo& info) {
void ThrowFatalError(const CallbackInfo& /*info*/) {
Error::Fatal("Error::ThrowFatalError", "This is a fatal error");
}

Expand Down
4 changes: 2 additions & 2 deletions test/external.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -15,7 +15,7 @@ Value CreateExternal(const CallbackInfo& info) {
Value CreateExternalWithFinalize(const CallbackInfo& info) {
finalizeCount = 0;
return External<int>::New(info.Env(), new int(1),
[](Env env, int* data) {
[](Env /*env*/, int* data) {
delete data;
finalizeCount++;
});
Expand All@@ -25,7 +25,7 @@ Value CreateExternalWithFinalizeHint(const CallbackInfo& info) {
finalizeCount = 0;
char* hint = nullptr;
return External<int>::New(info.Env(), new int(1),
[](Env env, int* data, char* hint) {
[](Env /*env*/, int* data, char* /*hint*/) {
delete data;
finalizeCount++;
},
Expand Down
Loading