-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
src: fixup strings, general code cleanup #14937
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -87,13 +87,22 @@ class AsyncWrap : public BaseObject { | |
PROVIDERS_LENGTH, | ||
}; | ||
|
||
enum Flags { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you’re passing it as ints, and since we have C++11 available: How about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wrote it this way to be consistent with StreamBase::Flags. Converting these to |
||
kFlagNone = 0x0, | ||
kFlagHasReset = 0x1 | ||
}; | ||
|
||
AsyncWrap(Environment* env, | ||
v8::Local<v8::Object> object, | ||
ProviderType provider, | ||
bool silent = false); | ||
|
||
virtual ~AsyncWrap(); | ||
|
||
static void AddWrapMethods(Environment* env, | ||
v8::Local<v8::FunctionTemplate> constructor, | ||
int flags = kFlagNone); | ||
|
||
static void Initialize(v8::Local<v8::Object> target, | ||
v8::Local<v8::Value> unused, | ||
v8::Local<v8::Context> context); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we bikeshed about the name.
IMHO
Add
->Bind
andWrap
->AsyncHooks
soBindAsyncHooksMethods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? I’d disagree.
Not the same things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disregard that, this is static method anyway, so it would always seem redundant.AddAsyncWrapMethods
might be more explicit, if you like it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I felt
AddWrapMethods
doesn't convey the semantics clearly, so I tried to decostruct, and see what felt wrong.Add
has (for me) the connotation of creating something new, and sinceenv->SetProtoMethod
AFAICT only creates JS binding for an existing native methodBind
sounds more appropriate. But we can also extend the existing metaphor and useSet
.Agreed, but AFAIK
getAsyncId
andasyncReset
are new and only used in the context of the hooks soAsyncHooks
felt more focused.Maybe
AsyncIDs
is even better.So to sum another option
SetAsyncIDsMethods
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On third thought
AsyncHooks
is not good, although that's the "context", the methods are only used by the hooks, and are not the hooks. so:+2 for
AsyncIDs
+1 for
AsyncWrap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm -1 on bikeshedding on this actually. Just don't see much point in doing so.