Project

General

Profile

Actions

Feature #14506

closed

Possible bug in cmdarg_stack

Added by ibylich (Ilya Bylich) about 6 years ago. Updated about 6 years ago.

Status:
Closed
Target version:
-
[ruby-core:85735]

Description

cmdarg stack is a stack of bytes that has 1 on top of it when the parser is in the command mode.

But for this code:

a b(c d), "x" do end

And this line in parse.y:

> SHOW_BITSTACK(p->cmdarg_stack, "BEFORE kDO_BLOCK");
if (CMDARG_P() && !IS_lex_state_for(state, EXPR_CMDARG))
  return keyword_do_block;

It prints:

$ dev-ruby -vye 'a b(c d), "x" do end' | grep -E "kDO_BLOCK"
BEFORE kDO_BLOCK: 10 at line 7461

So CMDARG_P is false. I understand that 'keyword_do_block" is handled later in the "if (IS_lex_state_for(state, (EXPR_BEG | EXPR_ENDARG)))" section, but is it possible to change parser to rely only on cmdarg stack?

Updated by mame (Yusuke Endoh) about 6 years ago

  • Status changed from Open to Assigned
  • Assignee set to nobu (Nobuyoshi Nakada)

I think that this is not a bug, and that your patch is for refactoring. Am I right?

Please clarify your motivation if any. I understand that you want this change for ease of backporting from parser.y to the parser gem. I'm neutral to this.

Anyway, your patch looks good to me as a refactoring. Nobu, what do you think?

Updated by ibylich (Ilya Bylich) about 6 years ago

This patch does not introduce any logical changes (at least I don't expect any).

mame (Yusuke Endoh) wrote:

I think that this is not a bug, and that your patch is for refactoring. Am I right?

Yes, exactly.

mame (Yusuke Endoh) wrote:

Please clarify your motivation if any. I understand that you want this change for ease of backporting from parser.y to the parser gem. I'm neutral to this.

Well, MRI's parser can rely only on cmdarg to recognize keyword_do/keyword_do_block, but for example above it doesn't. Instead, it transitions into dual state (which, from what I understand, works like a flag). Parser gem uses Ragel for lexing and Ragel doesn't support dual states.
The motivation of this patch is to refactor and simplify this place in MRI to allow backporting of this feature "as is" to the parser gem.

Updated by nobu (Nobuyoshi Nakada) about 6 years ago

Seems nice.
Maybe, lambda and brace_body parts aren't needed?

And, just curiosity, what do you mean by "dual state", cmdarg and lex_state?

Updated by ibylich (Ilya Bylich) about 6 years ago

nobu (Nobuyoshi Nakada) wrote:

Seems nice.
Maybe, lambda and brace_body parts aren't needed?

Maybe, I didn't have a chance to test it. Would you like me to check and revert this part?

nobu (Nobuyoshi Nakada) wrote:

And, just curiosity, what do you mean by "dual state", cmdarg and lex_state?

No, when lexer comes to the "do" token it has a state "EXPR_END|EXPR_ENDARG". I understand that it's a bitmask, but from what I see "EXPR_ENDARG" flag is needed only for emitting "do" as "keyword_do_block" (while originally cmdarg is designed to handle it and moreover, it can handle it on its own). Ragel doesn't support such states, in the parser gem it can be either EXPR_END or EXPR_ENDARG, but not both at the same time. That's why I have troubles with backporting it.

For MRI it's just a refactoring, for the parser gem it's (from what I see) the only way to backport it and keep semantical structure.

Updated by nobu (Nobuyoshi Nakada) about 6 years ago

  • Tracker changed from Bug to Feature
  • ruby -v deleted (ruby 2.6.0dev (2018-02-19 trunk 62484) [x86_64-darwin17])
  • Backport deleted (2.3: UNKNOWN, 2.4: UNKNOWN, 2.5: UNKNOWN)

ibylich (Ilya Bylich) wrote:

Maybe, lambda and brace_body parts aren't needed?

Maybe, I didn't have a chance to test it. Would you like me to check and revert this part?

No, just confirmed.

No, when lexer comes to the "do" token it has a state "EXPR_END|EXPR_ENDARG". I understand that it's a bitmask, but from what I see "EXPR_ENDARG" flag is needed only for emitting "do" as "keyword_do_block" (while originally cmdarg is designed to handle it and moreover, it can handle it on its own). Ragel doesn't support such states, in the parser gem it can be either EXPR_END or EXPR_ENDARG, but not both at the same time. That's why I have troubles with backporting it.

Thank you for clarification.

Actions #7

Updated by nobu (Nobuyoshi Nakada) about 6 years ago

  • Status changed from Assigned to Closed

Applied in changeset trunk|r62528.


Fix CMDARG manipulation

  • parse.y: Fix CMDARG manipulation. Use CMDARG_P to identify
    keyword_do/keyword_do_block. [Feature #14506] [Fix GH-1823]

From: Ilya Bylich

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0