Page MenuHomePhabricator

[LLDB][breakpoints] ArgInfo::count -> ArgInfo::max_positional_args

Authored by lawrence_danna on Oct 26 2019, 2:50 PM.



Move breakpoints from the old, bad ArgInfo::count to the new, better
ArgInfo::max_positional_args. Soon ArgInfo::count will be no more.

It looks like this functionality is already well tested by, so there's no need to write
additional tests for it.

Diff Detail

Event Timeline

lawrence_danna created this revision.Oct 26 2019, 2:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 26 2019, 2:50 PM
labath added inline comments.Oct 27 2019, 8:08 AM

Is there any case where fetching the argument info will fail, but we still can successfully call the target object? Should we just bail out here?

lawrence_danna marked an inline comment as done.Oct 27 2019, 10:54 AM
lawrence_danna added inline comments.

probably not? My thinking is that since there's no meaningful way to return an error from this function we may as well try to call it and let the exception get logged. But I dunno. Should I change it?

lawrence_danna marked an inline comment as done.Oct 27 2019, 4:10 PM
labath added inline comments.Oct 28 2019, 7:28 AM

Hmm... In that case, I think it would be better to print the error which caused the ArgInfo fetching to fail. Given that, in the new way of doing things, the PyErr_Cleaner object is not going to work anyway, we might as well use this opportunity to create a different mechanism for printing exceptions.

improved error handling

lawrence_danna marked an inline comment as done.Oct 28 2019, 1:15 PM
labath accepted this revision.Oct 29 2019, 12:13 AM

Looks fine, though I'd try to remove the extern "C" thingy to avoid the need to supress warnings and stuff...


How sure are we that these functions need to be extern "C" ? AFAICT, the only requirement is that they match the declarations in ScriptInterpreterPython.cpp. That can be easily achieved by just removing extern "C" from both declarations.


Random idea: Should we make PythonException::message() include the backtrace?

This revision is now accepted and ready to land.Oct 29 2019, 12:13 AM
lawrence_danna marked 4 inline comments as done.Oct 29 2019, 1:40 PM
lawrence_danna added inline comments.

Looks like SWIG forces them to be extern "C".


Huh, I don't know. Maybe?

lawrence_danna marked 2 inline comments as done.Oct 29 2019, 1:41 PM
This revision was automatically updated to reflect the committed changes.