search

arcalot / arcaflow-expressions

Expressions language for Arcaflow
Apache License 2.0
0 stars 0 forks source link

Refactor parseFunctionArgs and parseArgs #30

Open jaredoconnell opened 3 months ago

jaredoconnell commented 3 months ago 
          This would be a great place to handle the opening parenthesis, if it's present, _before_ calling `parseArgs()` (and then to handle the closing parenthesis, after it returns)...like it says in the function header comment.  😉

_Originally posted by @webbnh in https://github.com/arcalot/arcaflow-expressions/pull/23#discussion_r1471930600_

webbnh commented 3 months ago

Unfortunately, the hyperlink above to the comment is not working. I think this one does, though. However, the comment's link to the "earlier comment" doesn't work either; try this one instead. (The reasons why are a long story, the short of which is that I've reported this to GitHub as a bug.)

In any case, here's what the second comment says [slightly edited]:

I have a suggestion for reworking this function, which I think will make it simpler and easier to understand:

  • before entering the loop, look for and remove the opening parenthesis -- it's supposed to be there, and it's an error if it's not;
  • then, if the current token is not a closing paren, enter the loop (i.e., don't enter the loop if there are no arguments in the list); inside the loop, parse the argument; then exit the loop if the next token is not a comma; inside the loop, continue to remove the comma and iterate;
  • after the loop, look for and remove the closing parenthesis -- it's supposed to be there, and it's an error if it's not. This saves you from having to have a loop iterator and from having to have special cases for the first iteration of the loop. The only conditional is the exit condition in the middle of the loop if you encounter a closing paren instead of a comma after an argument.

If we were writing out the detailed grammar, it would look like this:

<function_call> := <identifier> <argument_list>
<argument_list> := "(" [ <arguments> ] ")"
<arguments> := <argument> [ "," <argument_list> ]

in specific, the opening and closing parens are part of the argument_list production but not part of the arguments production, and so they should be parsed independently of the arguments. (E.g., the loop should arguably be in a separate function.)

So, to summarize, it would be better if parseFunctionArgs() managed the opening and closing parentheses and parseArgs() managed only the sequence of zero or more arguments separated by commas and left everything else to its caller.