-
Notifications
You must be signed in to change notification settings - Fork 620
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
TypeScript: use CORK for scopes #2083
TypeScript: use CORK for scopes #2083
Conversation
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 wonder whether the condition if (scope)
is needed or not in a if (scope) dest->scope = foo
.
If it is redundant, we should remove the condition.
Could you look at lines setting the scope field?
parsers/typescript.c
Outdated
vStringCopy (methodScope, scope); | ||
vStringPut (methodScope, '.'); | ||
vStringCat (methodScope, member->string); | ||
const int nscope = isGenerator ? emitTag (member, TSTAG_GENERATOR) : emitTag (member, TSTAG_METHOD); |
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.
const int nscope = emitTag (member, (isGenerator? TSTAG_GENERATOR: TSTAG_METHOD));
reduces typings :-P
parsers/typescript.c
Outdated
vStringCopy (token->scope, scope); | ||
token->scopeParentKind = scopeParentKind; | ||
} | ||
if (scope != CORK_NIL) token->scope = scope; |
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.
Do we need the condtion?
I'm not sure but I think
token->scope = scope;
is o.k. even if scope == CORK_NIL
.
parsers/typescript.c
Outdated
vStringCopy (token->scope, scope); | ||
token->scopeParentKind = scopeParentKind; | ||
} | ||
if (scope != CORK_NIL) token->scope = scope; |
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.
The last comment may be applicable to this line, too.
parsers/typescript.c
Outdated
vStringPut (nscope, '.'); | ||
} | ||
vStringCat (nscope, token->string); | ||
if (scope != CORK_NIL) token->scope = scope; |
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.
The last comment is applicable to this line.
parsers/typescript.c
Outdated
vStringCopy (member->scope, scope); | ||
member->scopeParentKind = scopeParentKind; | ||
} | ||
if (scope != CORK_NIL) member->scope = scope; |
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.
The last comment is applicable to this line.
parsers/typescript.c
Outdated
vStringCopy (token->scope, scope); | ||
token->scopeParentKind = scopeParentKind; | ||
} | ||
if (scope != CORK_NIL) token->scope = scope; |
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.
The last comment is applicable to this line.
parsers/typescript.c
Outdated
emitTag (token, TSTAG_GENERATOR); | ||
else | ||
emitTag (token, TSTAG_FUNCTION); | ||
const int nscope = isGenerator ? emitTag (token, TSTAG_GENERATOR) : emitTag (token, TSTAG_FUNCTION); |
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.
This can be shorter with:
const int nscope = emitTag (token, isGenerator ? TSTAG_GENERATOR: TSTAG_FUNCTION);
.
parsers/typescript.c
Outdated
vStringPut (nscope, '.'); | ||
} | ||
vStringCat (nscope, token->string); | ||
if (scope != CORK_NIL) token->scope = scope; |
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.
Do we need the condition?
I would like to write about a little bit away from the topic of this pull request. Did you try
By setting
|
I made some minor comments. Other than them, It looks good to me. |
Thanks @masatake! I'll fix them. |
I think I've addressed all the issues as well as automatic fq tags. |
a17c29f
to
4f105fb
Compare
@masatake Done. Is it now OK? |
Sorry, I have one more comment. |
I added |
Wow, sorry. I missed the test case. I will take asleep:-). |
OK, thanks! |
Thank you. |
No big perfomance degrading with introducing CORK:
|
I've changed manually handling scopes as strings in favor of CORK indexes. Parser code is now shorter and more clear. I've also introduced myself in developers.rst