Fix Ruby parsing for block assigns #1732
Conversation
Assigning blocks' return values to variables is a common idiom in Ruby. Fix the parser to take this into account.
|
Could you provide test cases for this? Also, test cases for your TODOs would be nice too and help future work. |
After entering the `while` block parsing the line character by character, we don't need to use the previously introduced canMatchKeywordWithAssign function.
A test case assigning various blocks to variables, for exercising the fix introduced in 6c90039.
|
OK, for now I added a test case for determining if the correct tags are extracted if blocks are assigned to variables. |
|
@tambeta, Thank you. Could you paste here an example input that uses |
| method_a input.rb /^ x = def method_a$/;" f class:Bar | ||
| method_b input.rb /^ def method_b$/;" f class:Bar | ||
| method_c input.rb /^ def method_c$/;" f class:Bar | ||
| method_d input.rb /^ def method_d$/;" f class:Bar |
There was a problem hiding this comment.
Shouldn't m, c and x be reported as something (variables I guess)?
I'm not sure where this syntax is used, but apparently it does something (store the result of the last statement? not sure from experimenting…), so I guess they should ideally be reported, shouldn't they?
There was a problem hiding this comment.
They could be reported as variables, but I believe this hasn't been the case thus far. Anyway, it isn't in the scope of this PR, as it just aims to fix the annoying bug where class members are reported as free-floating methods if block assignment is used.
A failing test case using shortcut assignment operators with blocks, serving as a TODO for improving on 6c90039.
|
@masatake, I took the time to read about the protocol for adding known bug tests and the latest commit adds a failing test where various "shortcut" assignment operators are used ( |
Extend block assignment parsing by allowing "shortcut assignment" operators, e.g. `||=`, `+=`.
|
Giving it some thought, it wasn't a big job to extend parsing to allow "shortcut" assignment operators. The latest commit introduces this. |
parsers/ruby.c
Outdated
| */ | ||
| static bool advanceWhile (const unsigned char** s, bool (*predicate) (int)) | ||
| { | ||
| const int s_length = strlen ((const char *)*s); |
parsers/ruby.c
Outdated
| unsigned char this_char; | ||
| int i = 0; | ||
|
|
||
| for (i = 0; i < s_length; ++i) |
There was a problem hiding this comment.
This function scans the input twice: once by strlen and once by this for.
How about replacing them with following while?
while (**s != `\0`)
parsers/ruby.c
Outdated
| @@ -167,7 +235,7 @@ static bool parseRubyOperator (vString* name, const unsigned char** cp) | |||
| { | |||
| if (canMatch (cp, RUBY_OPERATORS[i], notOperatorChar)) | |||
| { | |||
| vStringCatS (name, RUBY_OPERATORS[i]); | |||
| vStringCatS (name, RUBY_OPERATORS[i]); | |||
There was a problem hiding this comment.
Could you keep this line as the orignal?
|
I made some minor comments but it looks good to me. |
|
Noted and implemented. |
|
Thank you. |
|
Could you look at #1734. To make the history of commits simple, I rearranged your changes. |
|
OK, let's take the discussion over there. |
|
I merged your work in #1734. Thank you. |
|
Great, thanks. |
Assigning blocks' return values to variables is a common idiom in Ruby. Fix the parser to take this into account.
I'm not sure if matching against regexen would be acceptable in ctags, but this would certainly make things easier. The proposed implementation doesn't use regexps.
I originally submitted this fix to Geany, which includes ctags as part of its source tree - Geany devs recommended posting it to upstream first. The issue is explained over there.