Skip to content

Fix a -Wdeprecated-declarations warning.#20

Merged
cheery merged 1 commit intocheery:masterfrom
NetOxygen:deprecated-warning-fix
Jun 9, 2017
Merged

Fix a -Wdeprecated-declarations warning.#20
cheery merged 1 commit intocheery:masterfrom
NetOxygen:deprecated-warning-fix

Conversation

@kaworu
Copy link
Copy Markdown
Contributor

@kaworu kaworu commented Jun 9, 2017

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

This patch remove the declaration as the variable is unused.
@cheery
Copy link
Copy Markdown
Owner

cheery commented Jun 9, 2017

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
Copy link
Copy Markdown
Contributor Author

kaworu commented Jun 9, 2017

@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
Copy link
Copy Markdown
Owner

cheery commented Jun 9, 2017

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.

@cheery cheery merged commit dfaba2f into cheery:master Jun 9, 2017
@kaworu kaworu deleted the deprecated-warning-fix branch June 9, 2017 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants