-
-
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
semantic,codegen: allow optional positional params #742
Conversation
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 some tests to make sure we don't delete this functionality again
21b66ff
to
5ebba50
Compare
Added some tests. Had to make a few changes in some headers to avoid circular include issues. Please take a look. |
5ebba50
to
0fe0a5c
Compare
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.
Maybe bpftrace.get_param()
could contain the default value logic instead? Might need an extra argument like is_in_str
src/ast/ast.cpp
Outdated
@@ -29,6 +30,20 @@ void PositionalParameter::accept(Visitor &v) { | |||
v.visit(*this); | |||
} | |||
|
|||
std::string PositionalParameter::get_value(const BPFtrace &bpftrace) |
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.
Don't think this method should be part of the AST - this class shouldn't know about bpftrace at all
0fe0a5c
to
c7a7c87
Compare
Adding a new parameter to |
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!
Fixes: #735