-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Python: Move min/maxParameter methods to Function
class
#18871
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
Function
These seem generally useful outside of points-to, and so it might be better to add them to the `Function` class instead. I took the liberty of renaming these to say `Arguments` rather than `Parameters`, as this is more in line with the nomenclature that we're using elsewhere. (The internal points-to methods retain the old names.) I'm somewhat ambivalent about the behaviour of `getMaxParameters` on functions with `*varargs`. The hard-coded `INT_MAX` return value is somewhat awkward, but the alternative (to only have the predicate defined when a specific maximum exists) seems like it would potentially cause a lot of headaches.
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -163,6 +163,18 @@ class Function extends Function_, Scope, AstNode { | |
ret.getValue() = result.getNode() | ||
) | ||
} | ||
|
||
/** Gets the minimum number of positional arguments that can be correctly passed to this function. */ | ||
int getMinArguments() { | ||
result = count(this.getAnArg()) - count(this.getDefinition().getArgs().getADefault()) | ||
} | ||
|
||
/** Gets the maximum number of positional arguments that can be correctly passed to this function. */ | ||
int getMaxArguments() { | ||
if exists(this.getVararg()) | ||
then result = 2147483647 // INT_MAX | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ooh this is an interesting trap. I suggest at least putting it in the QLDoc, i.e. if the function doesn't take varargs, the result is INT_MAX. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you've got a superfluous negation there (if the function does take varargs, the result is INT_MAX), but I agree this should be documented better. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 11e6194. |
||
else result = count(this.getAnArg()) | ||
} | ||
} | ||
|
||
/** A def statement. Note that FunctionDef extends Assign as a function definition binds the newly created function */ | ||
|
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.
Naming: Should these be
getMinPositionalArguments
, something like that?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.
It's a mouthful (and it suggests the existence of
getMinKeywordArguments
, which I'm not entirely sure makes sense), but it's probably a good idea to proactively disambiguate things here. At the very least, renaming it to something shorter will be easy to do if we feel like it.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.
Done in 11e6194.