Add a workaround for people that use *args instead of listing#166883
Add a workaround for people that use *args instead of listing#166883
Conversation
parameters when defining the scripting interfaces. We try to count the parameters to make sure the user has defined them correctly, but this throws the counting off. I'm not adding a test for this because then it would seem like we thought this was a good idea. I'd actually rather not support it altogether, but we added the parameter checking pretty recently so there are extant implementations that we broke. I only want to support them, not suggest anyone else do this going forward.
|
@llvm/pr-subscribers-lldb Author: None (jimingham) Changesparameters when defining the scripting interfaces. We try to count the parameters to make sure the user has defined them correctly, but this throws the counting off. I'm not adding a test for this because then it would seem like we thought this was a good idea. I'd actually rather not support it altogether, but we added the parameter checking pretty recently so there are extant implementations that we broke. I only want to support them, not suggest anyone else do this going forward. Full diff: https://github.com/llvm/llvm-project/pull/166883.diff 1 Files Affected:
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h
index 2335b2ef0f171..31f38444b04cd 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h
@@ -188,8 +188,13 @@ class ScriptedPythonInterface : virtual public ScriptedInterface {
// This addresses the cases where the embedded interpreter session
// dictionary is passed to the extension initializer which is not used
// most of the time.
+ // Note, though none of our API's suggest defining the interfaces with
+ // varargs, we have some extant clients that were doing that. To keep
+ // from breaking them, we just say putting a varargs in these signatures
+ // turns off argument checking.
size_t num_args = sizeof...(Args);
- if (num_args != arg_info->max_positional_args) {
+ if (arg_info->max_positional_args != PythonCallable::ArgInfo::UNBOUNDED
+ && num_args != arg_info->max_positional_args) {
if (num_args != arg_info->max_positional_args - 1)
return create_error("Passed arguments ({0}) doesn't match the number "
"of expected arguments ({1}).",
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| // turns off argument checking. | ||
| size_t num_args = sizeof...(Args); | ||
| if (num_args != arg_info->max_positional_args) { | ||
| if (arg_info->max_positional_args != PythonCallable::ArgInfo::UNBOUNDED && |
There was a problem hiding this comment.
If we wanted to discourage this pattern, we could emit a warning with Debugger::ReportWarning. It takes an optional once_flag that would allow us to limit this warning to once per ScriptedPythonInterface instance or even once per lldb instance if we made it static.
There was a problem hiding this comment.
I don't think that's necessary. Someone did this once and then sadly that got copied around and so there are a few versions I need to maintain support for. I'm not sure this is something anyone else is likely to think is a good idea.
4 participants
parameters when defining the scripting interfaces.
We try to count the parameters to make sure the user has defined them correctly, but this throws the counting off.
I'm not adding a test for this because then it would seem like we thought this was a good idea. I'd actually rather not support it altogether, but we added the parameter checking pretty recently so there are extant implementations that we broke. I only want to support them, not suggest anyone else do this going forward.