I've recently committed two patches:
- commit 2f8cd40c37 ("[gdb/python] Use GDB_PY_HANDLE_EXCEPTION more often")
- commit fbf8e4c35c ("[gdb/python] Use GDB_PY_SET_HANDLE_EXCEPTION more often")
which use the macros GDB_PY_HANDLE_EXCEPTION and GDB_PY_SET_HANDLE_EXCEPTION
more often, with the goal of making things more consistent.
Having done that, I wondered if a better approach could be possible.
Consider GDB_PY_HANDLE_EXCEPTION:
...
/* Use this in a 'catch' block to convert the exception to a Python
exception and return nullptr. */
#define GDB_PY_HANDLE_EXCEPTION(Exception) \
do { \
gdbpy_convert_exception (Exception); \
return nullptr; \
} while (0)
...
The macro nicely codifies how python handles exceptions:
- setting an error condition using some PyErr_Set* variant, and
- returning a value implying that something went wrong
presumably with the goal that using the macro will mean not accidentally:
- forgetting to return on error, or
- returning the wrong value on error.
The problems are that:
- the macro hides control flow, specifically the return statement, and
- the macro hides the return value.
For example, when reading somewhere:
...
catch (const gdb_exception &except)
{
GDB_PY_HANDLE_EXCEPTION (except);
}
...
in order to understand what this does, you have to know that the macro
returns, and that it returns nullptr.
Add a template gdbpy_handle_gdb_exception:
...
template<typename T>
[[nodiscard]] T
gdbpy_handle_gdb_exception (T val, const gdb_exception &e)
{
gdbpy_convert_exception (e);
return val;
}
...
which can be used instead:
...
catch (const gdb_exception &except)
{
return gdbpy_handle_gdb_exception (nullptr, except);
}
...
[ Initially I tried this:
...
template<auto val>
[[nodiscard]] auto
gdbpy_handle_gdb_exception (const gdb_exception &e)
{
gdbpy_convert_exception (e);
return val;
}
...
with which the usage is slightly better looking:
...
catch (const gdb_exception &except)
{
return gdbpy_handle_gdb_exception<nullptr> (except);
}
...
but I ran into trouble with older gcc compilers. ]
While still a single statement, we now have it clear:
- that the statement returns,
- what value the statement returns.
[ FWIW, this could also be handled by say:
...
- GDB_PY_HANDLE_EXCEPTION (except);
+ GDB_PY_HANDLE_EXCEPTION_AND_RETURN_VAL (except, nullptr);
...
but I still didn't find the fact that it returns easy to spot.
Alternatively, this is the simplest form we could use:
...
return gdbpy_convert_exception (e), nullptr;
...
but the pairing would not necessarily survive a copy/paste/edit cycle. ]
Also note how making the value explicit makes it easier to check for
consistency:
...
catch (const gdb_exception &except)
{
return gdbpy_handle_gdb_exception (-1, except);
}
if (PyErr_Occurred ())
return -1;
...
given that we do use the explicit constants almost everywhere else.
Compared to using GDB_PY_HANDLE_EXCEPTION, there is the burden now to specify
the return value, but I assume that this will be generally copy-pasted and
therefore present no problem.
Also, there's no longer a guarantee that there's an immediate return, but I
assume that nodiscard making sure that the return value is not silently
ignored is sufficient mitigation.
For now, re-implement GDB_PY_HANDLE_EXCEPTION and GDB_PY_SET_HANDLE_EXCEPTION
in terms of gdbpy_handle_gdb_exception.
Follow-up patches will eliminate the macros.
No functional changes.
Tested on x86_64-linux.
Approved-By: Tom Tromey <tom@tromey.com>
912bc231ab