cheery / node-udev

Bindings to libudev
35 stars 31 forks source link

Fix a -Wdeprecated-declarations warning. #20

Closed kaworu closed 7 years ago

kaworu commented 7 years ago

This patch remove the declaration as the variable is unused.

Before this PR we get:

  CXX(target) Release/obj.target/udev/udev.o
../udev.cc: In static member function ‘static void Monitor::on_handle_event(uv_poll_t*, int, int)’:
../udev.cc:62:18: warning: ‘v8::TryCatch::TryCatch()’ is deprecated: Use isolate version [-Wdeprecated-declarations]
         TryCatch tc;
                  ^

System info for the context:

% node --version
v6.10.3
% npm --version
5.0.3
% lsb_release -d                                                                                                                       
Description:    Ubuntu 16.04.2 LTS
cheery commented 7 years ago

Hi @kAworu and thanks for the patches.

Are the exceptions correctly logged and handled despite removing this? Or are they handled in the first place? Could you check that out?

kaworu commented 7 years ago

@cheery my pleasure for the patches, thank you for this module! We are using it @Netoxygen on Raspberry Pi to detect some type of devices connected with great success.

About this PR, I don't see any changes in behaviour (exceptions are not logged nor handled) before and after the patch. I also tried to replace it by a Nan::TryCatch but without any visible impact either, so finally choose to just remove it instead.

I feel I must be testing it incorrectly, here is an example script I used:

var util = require("util");
var udev = require("./udev.js");

var monitor = udev.monitor();
monitor.on('add', function (device) {
    console.log(device.DEVPATH + " added");
    throw new Error("Trycatch?");
});

Does this example looks like a good test case to you? If not, can you provide an example or hints? If yes, what is the expected behaviour?

cheery commented 7 years ago

If the error doesn't appear like it appears in this case:

setTimeout(function(){
    console.log("timeout");
    throw new Error("Trycatch?");
}, 1);

Then we got a bug there. It's not a big thing but it ensures you get some message into the log if there's a problem.

To be consistent, it may be good idea to check from some other library how they do this. It might even be a common usage pattern of Local that we are missing here.

All right.. I merge this.