-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add $# for number of positional arguments #791
Conversation
I just rebased this on top of #742. This required reviving |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Just left some minor comments 😄
src/ast/printer.cpp
Outdated
|
||
switch (param.ptype) { | ||
case PositionalParameterType::positional: | ||
out_ << indent << "param: $" << param.n<< std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: space after param.n
@@ -104,6 +103,8 @@ bpftrace|perf { | |||
"~" { return Parser::make_BNOT(loc); } | |||
"." { return Parser::make_DOT(loc); } | |||
"->" { return Parser::make_PTR(loc); } | |||
"$"[0-9]+ { return Parser::make_PARAM(yytext, loc); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will match 0000001
, for example
Maybe "$"[0-9][1-9]*
is better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, what I have is consistent with the current code and 001
is arguably as valid an integer as 1
so I think it'll just be a matter of preference. I'm fine to change to "$"[1-9][0-9]*
if that's the consensus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just need to update the reference guide after that last rebase
docs/reference_guide.md
Outdated
|
||
These are the positional parameters to the bpftrace program, also referred to as command line arguments. If the parameter is numeric (entirerly digits), it can be used as a number. If it is non-numeric, it must be used as a string in the `str()` call. If a parameter is used that was not provided, it will default to zero for numeric context, and "" for string context. | ||
These are the positional parameters to the bpftrace program, also referred to as command line arguments. If the parameter is numeric (entirely digits), it can be used as a number. If it is non-numeric, it must be used as a string in the `str()` call. Referring to parameters that were not supplied is not permitted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last sentence here should go back to the old version now that optional parameters are supported again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay, yes please
PS, this has a conflict that needs resolving (do you see a "Resolve conflicts" button as well? hopefully it automates it and makes it easy) |
The latest commit should fix the reference guide and rebase onto trunk. I also dropped a scope I had unnecessarily introduced into semantic_analyser.cpp (7e387b2#diff-286594496696c8553c9f12fcabfb5f38). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This PR adds a parameter,
$#
, to return the number of positional parameters passed.I rejigged the existing parameter matching because trying to match
$
followed by#
doesn't work (the scanner matches the#
as a C preprocessor token). I put the parameter count stuff intoPositionalParameter
but could probably separate it if needed. I also updated the positional parameter help to reflect #717.I'm pretty new to all of this so let me know if there are obvious errors/bad names/poor formatting/etc.
Fixes: #741