Default initialize function argument, closes #1090#1092
Default initialize function argument, closes #1090#1092hsutter merged 2 commits intohsutter:mainfrom
Conversation
70670c3 to
c181fb1
Compare
|
Looks good, a regression test would be nice. |
Yeah, it passed on my fork but it looks like the tests don't run on their own in this repo? |
https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks
|
Ah, what I meant, is that it would be nice if you add a regression test for the new feature. Sorry for the misunderstanding. |
|
Cool, added a regression test. Let me know what you think. |
MaxSagebaum
left a comment
There was a problem hiding this comment.
Nice, thanks for adding the test. It looks good to me. I just have a minor comment.
| is_inside_call_expr = true; | ||
|
|
||
| term.expr_list = expression_list(term.op, lexeme::RightParen); | ||
|
|
||
| is_inside_call_expr = false; | ||
|
|
There was a problem hiding this comment.
I thought, that it might be better to add is_inside_call_expr as an argument. But I think from the geeneral design it is not possible. So the state modification is ok. In order to make the more prominent, I would changed it to:
| is_inside_call_expr = true; | |
| term.expr_list = expression_list(term.op, lexeme::RightParen); | |
| is_inside_call_expr = false; | |
| is_inside_call_expr = true; // Flag this expression list parse. | |
| term.expr_list = expression_list(term.op, lexeme::RightParen); | |
| is_inside_call_expr = false; | |
There was a problem hiding this comment.
That makes sense. Done.
There was a problem hiding this comment.
Right, it's either use out-of-band state, or wire a parameter through the call chain. Sometimes I do the former, sometimes the latter, depending on the situation.
|
Thanks! |
@hsutter @bluetarpmedia
This patches #1090 by allowing defaulting of arguments in a function call by use of '()' (akin to '{}' in C++).