Skip to content

Conversation

@skovhus
Copy link
Contributor

@skovhus skovhus commented May 16, 2020

Work in progress.

While browsing around in the known-errors (bash-it) I figured out that the C-style for-loops support has several bugs:

  • does not support multiple variables assignments or updates (fixed)
  • does not support spaces around variable assignments for ((x = 0 ; x <= 100 ; x++)); is not parsed as an assignment, but a command with an error (not fixed)

@maxbrunsfeld I'm not sure how I should approach the last issue. It seems variable assignment support inside for loops might be different than normal assignments. Do I have any control over white space parsing inside grammar.js?

See https://wiki-dev.bash-hackers.org/syntax/ccmd/c_for for context

@skovhus
Copy link
Contributor Author

skovhus commented May 17, 2020

I see now that there is some overlap with #55

@skovhus
Copy link
Contributor Author

skovhus commented May 29, 2020

@maxbrunsfeld Let me know if you have any comments.

@maxbrunsfeld
Copy link
Contributor

It seems variable assignment support inside for loops might be different than normal assignments.

Yeah, it appears so. I think x = y will be handled currently by the binary_expression rule. Maybe we just need to add variable_assignment as an additional choice for the initializer (instead of replacing _expression with only variable_assignment).

Also, I don't think we should use repeat($._expression), because that would mean you could have multiple expressions immediately following one another, like i++ b++. In reality, there needs to be a comma in between.

@skovhus
Copy link
Contributor Author

skovhus commented May 29, 2020

Thanks. I’ll have another look.

@skovhus skovhus force-pushed the c-style-for-loops branch from 570248f to 3294562 Compare July 4, 2020 19:12
Variable assignment syntax is more complicated though...
@skovhus skovhus force-pushed the c-style-for-loops branch from 3294562 to 7f8e2c6 Compare March 27, 2021 21:34
@skovhus skovhus closed this Nov 1, 2021
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