Skip to content
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

inspector: move options parsing #9691

Merged
merged 0 commits into from
Dec 9, 2016
Merged

inspector: move options parsing #9691

merged 0 commits into from
Dec 9, 2016

Conversation

eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Nov 19, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

inspector, core - some code was copied from the core to the inspector.

Description of change

As inspector functionality expands, more options will need to be added.
Currently this requires changing adding function arguments, etc. This
change packs the veriables into a single class that can be extended
without changing APIs.

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 19, 2016
@eugeneo
Copy link
Contributor Author

eugeneo commented Nov 19, 2016

@@ -1,5 +1,9 @@
#include "inspector_agent.h"

#if !HAVE_INSPECTOR
#error("This file can only be used when the inspector is enabled")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do this in the header file?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant, inspector_agent.h

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I changed the code to process both "debugger" and inspector properties, now it lives in a dedicated header - I restored the check in the inspector_agent.h

bool consume = false;
const char* port = nullptr;

if (!strncmp(option, "--debug=", sizeof("--debug=") - 1)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer strlen over sizeof(..) - 1. Is sizeof better than strlen here?

Copy link
Member

Choose a reason for hiding this comment

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

It's what we use everywhere else for static strings so this looks alright me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thefourtheye strlen is evaluated dynamically while sizeof is evaluated at compile time. I don't think this yields any perceivable gains, just the force of habit :)

port = option + sizeof("--debug-port=") - 1;
// Specifying both --inspect and --debug means debugging is on, using Chromium
// inspector.
} else if (!strcmp(option, "--inspect")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not strncmp here?

Copy link
Member

Choose a reason for hiding this comment

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

It's matching against the whole string instead of just a slice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I redid the code.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Duplicating a sizable chunk of fairly subtle code from src/node.cc is not great, IMO. Is there a way to do better?

bool consume = false;
const char* port = nullptr;

if (!strncmp(option, "--debug=", sizeof("--debug=") - 1)) {
Copy link
Member

Choose a reason for hiding this comment

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

It's what we use everywhere else for static strings so this looks alright me.

port = option + sizeof("--debug-port=") - 1;
// Specifying both --inspect and --debug means debugging is on, using Chromium
// inspector.
} else if (!strcmp(option, "--inspect")) {
Copy link
Member

Choose a reason for hiding this comment

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

It's matching against the whole string instead of just a slice.

bool InspectorEnabled() const { return HttpEnabled(); }
bool HttpEnabled() const { return http_enabled_; }
bool WaitForConnect() const { return wait_connect_; }
std::string HttpHost() const { return host_; }
Copy link
Member

Choose a reason for hiding this comment

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

I realize this follows existing convention but HttpHost() is something of a misnomer since it's an address, not a free-form host name.

Aside: simple getters are usually called after the property they encapsulate minus the trailing underscore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@eugeneo
Copy link
Contributor Author

eugeneo commented Nov 22, 2016

@bnoordhuis Thank you for the review. I redid the change to avoid duplication, please take another look.

} else if (option_name == "--debug-port" && has_argument) {
// Do nothing - argument will be parsed below
// Specifying both --inspect and --debug means debugging is on, using Chromium
// inspector.
Copy link
Member

Choose a reason for hiding this comment

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

Indentation is off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

}
} else {
const int skip =
(colon > 0 && port[0] == '[' && port[colon - 1] == ']') ? 1 : 0;
Copy link
Member

Choose a reason for hiding this comment

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

The ternary is superfluous here, bools decay to ints, and it makes it harder to read, IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rearranged the code a bit.

return DebugAgentType::kDebugger;
}
return DebugAgentType::kNone;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you add bool IsInspector() const convenience methods? That would let you simplify conditionals in other files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did that.

bool WaitForConnect() const { return wait_connect_; }
std::string HostName() const { return host_name_; }
int Port() const;
void SetPort(int port) { port_ = port; }
Copy link
Member

Choose a reason for hiding this comment

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

It's not a big thing but I would call these hostname(), port() and set_port(). The sort-of convention in core is that CamelCase methods imply computation or side effects whereas snake_case methods are simple getters and setters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@eugeneo
Copy link
Contributor Author

eugeneo commented Nov 23, 2016

@bnoordhuis Thank you for the review. Uploaded a newer version of the patch, please take another look.

@eugeneo
Copy link
Contributor Author

eugeneo commented Dec 1, 2016

@bnoordhuis Please take a look. CI is passing: https://ci.nodejs.org/job/node-test-pull-request/5022/

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Left some suggestions. Mostly LGTM except for the !host.empty() thing, that looks like a bug or at least a very subtle foot gun.

#include "node_debug_options.h"

#include <errno.h>
#include <string.h>
Copy link
Member

Choose a reason for hiding this comment

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

You should also include stdlib.h for the definition of exit() and strtol() and stdio.h for fprintf().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

const int default_inspector_port = 9229;

inline std::string remove_brackets(const std::string& host) {
if (host.front() == '[' && host.back() == ']')
Copy link
Member

Choose a reason for hiding this comment

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

!host.empty() first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks for pointing this out.

} else {
return std::make_pair(remove_brackets(arg.substr(0, colon)),
parse_and_validate_port(arg.substr(colon + 1)));
}
Copy link
Member

Choose a reason for hiding this comment

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

If you omit the else, you can save a level of indent here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

std::string argument;

auto pos = option.find("=");
if (pos != std::string::npos) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd swap the clauses; x == y is infinitesimally easier to parse for most people and it puts the shortest block first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@eugeneo
Copy link
Contributor Author

eugeneo commented Dec 1, 2016

@bnoordhuis Thank you for the review, I uploaded the new revision.

@eugeneo
Copy link
Contributor Author

eugeneo commented Dec 6, 2016

Rebased on top of recent changes. This code is now ready for the review.

@eugeneo
Copy link
Contributor Author

eugeneo commented Dec 9, 2016

Landed as f9aadfb

@eugeneo eugeneo deleted the args branch December 9, 2016 17:10
@eugeneo eugeneo merged commit f9aadfb into nodejs:master Dec 9, 2016
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
As inspector functionality expands, more options will need to be added.
Currently this requires changing adding function arguments, etc. This
change packs the veriables into a single class that can be extended
without changing APIs.

PR-URL: nodejs#9691
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this pull request Jan 28, 2017
As inspector functionality expands, more options will need to be added.
Currently this requires changing adding function arguments, etc. This
change packs the veriables into a single class that can be extended
without changing APIs.

PR-URL: #9691
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
As inspector functionality expands, more options will need to be added.
Currently this requires changing adding function arguments, etc. This
change packs the veriables into a single class that can be extended
without changing APIs.

PR-URL: nodejs#9691
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
As inspector functionality expands, more options will need to be added.
Currently this requires changing adding function arguments, etc. This
change packs the veriables into a single class that can be extended
without changing APIs.

PR-URL: nodejs#9691
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants