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

JavaScript: allow class methods get and set #3329

Conversation

jafl
Copy link
Contributor

@jafl jafl commented Apr 7, 2022

get and set are used to define getters and setters for a class. However, get and set can also be method names. This patch adds support for the latter.

gnulib/.gitignore Outdated Show resolved Hide resolved
@masatake
Copy link
Member

masatake commented Apr 7, 2022

Could you remove the last commit 0b5c16a?

Saying frankly, I don't understand what it is. However, I guess, it is not a "must" for the purpose of this pull request.
If I'm wrong, please let me know.

@masatake masatake requested a review from b4n April 7, 2022 20:55
@masatake
Copy link
Member

masatake commented Apr 7, 2022

@b4n, IF you have time, I would like to review this pull request.

@jafl, thank you.

@codecov
Copy link

codecov bot commented Apr 7, 2022

Codecov Report

Merging #3329 (0b5c16a) into master (c4e3847) will decrease coverage by 0.00%.
The diff coverage is 70.83%.

❗ Current head 0b5c16a differs from pull request most recent head 661398c. Consider uploading reports for the commit 661398c to get more accurate results

@@            Coverage Diff             @@
##           master    #3329      +/-   ##
==========================================
- Coverage   85.44%   85.43%   -0.01%     
==========================================
  Files         216      216              
  Lines       50319    50337      +18     
==========================================
+ Hits        42993    43004      +11     
- Misses       7326     7333       +7     
Impacted Files Coverage Δ
parsers/jscript.c 96.58% <70.83%> (-0.60%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4e3847...661398c. Read the comment docs.

parsers/jscript.c Outdated Show resolved Hide resolved
@jafl jafl force-pushed the javascript-allow-class-methods-get-and-set branch 2 times, most recently from 0d1bd8d to 6bcd629 Compare April 7, 2022 22:24
@jafl
Copy link
Contributor Author

jafl commented Apr 7, 2022

Once everything else looks good, I will include the .gitignore for gnulib, with a newline on the end, as a separate commit.

@jafl jafl changed the title Javascript: allow class methods get and set JavaScript: allow class methods get and set Apr 7, 2022
@masatake
Copy link
Member

masatake commented Apr 8, 2022

How about the following change?

A. if the token just read is "TOKEN_OPEN_PAREN", NextToken must be NULL.
Based on this assumption, we can remove 'NextToken == NULL' from the condition.

B. this parser assumes if(isKeyword(token, ...) ) {... has the same meaning if (isType (token, TOKEN_KEYWORD) && isKeyword(token, ...) {.... Therefore, if you assign non-TOKEN_KEYWORD to token->type, you may have to set KEYWORD_NONE to token->keyword.

diff --git a/parsers/jscript.c b/parsers/jscript.c
index a1be93576..1b8bc3108 100644
--- a/parsers/jscript.c
+++ b/parsers/jscript.c
@@ -1659,13 +1659,14 @@ static bool parseMethods (tokenInfo *const token, const tokenInfo *const class,
 			tokenInfo *savedToken = newToken ();
 			copyToken (savedToken, token, true);
 			readToken (token);
-			if (isType(token, TOKEN_OPEN_PAREN) && NextToken == NULL)
+			if (isType(token, TOKEN_OPEN_PAREN))
 			{
 				Assert (NextToken == NULL);
 				NextToken = newToken ();
 				copyToken (NextToken, token, false);	/* save token for next read */
 				copyToken (token, savedToken, true);	/* restore token to process */
 				token->type = TOKEN_IDENTIFIER;			/* process as identifier */
+				token->keyword	= KEYWORD_NONE;
 			}
 			else if (isKeyword (savedToken, KEYWORD_get))
 			{

@jafl jafl force-pushed the javascript-allow-class-methods-get-and-set branch from 6bcd629 to 2c210f9 Compare April 8, 2022 13:49
@jafl
Copy link
Contributor Author

jafl commented Apr 8, 2022

Thanks for catching that!

@jafl jafl force-pushed the javascript-allow-class-methods-get-and-set branch from 2c210f9 to 661398c Compare April 9, 2022 21:16
@masatake
Copy link
Member

masatake commented Apr 9, 2022

Could you wait for a week till merging?
I wonder whether we can get a review from the main developer of the parser.

@masatake masatake self-requested a review April 13, 2022 16:02
@jafl
Copy link
Contributor Author

jafl commented Apr 13, 2022

@masatake Note that I cannot merge any pull requests. You will have to do it when you feel comfortable.

@masatake
Copy link
Member

@jafl, yes, I know.

I approved your pull request explicitly before merging just for recording what I did as the maintainer of this repository. I will merge your pull request this weekend.

@masatake masatake merged commit fc561fb into universal-ctags:master Apr 15, 2022
@masatake
Copy link
Member

Thank you.

@jafl jafl deleted the javascript-allow-class-methods-get-and-set branch April 17, 2022 20:23
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