RE: [PATCH v10 05/10] python: Introduce gdb.RecordAuxiliary class.

Metzger, Markus T via Gdb-patches
2023-07-25T13:30:34Z

Hello Felix,

>- btrace_find_insn_by_number (&iter, &tinfo->btrace, number);
>+ if (btrace_find_insn_by_number (&iter, &tinfo->btrace, number) == 0)

Nit: how about storing the result in a local variable to avoid side-effects
in the if expression?

>+ if (Py_TYPE (self) != &recpy_aux_type)
>+ {
>+ PyErr_Format (gdbpy_gdb_error, _("Must be a gdb.Auxiliary."));
>+ return NULL;
>+ }
>+
>+ obj = (const recpy_element_object *) self;
>+ tinfo = obj->thread;
>+
>+ if (tinfo == NULL || btrace_is_empty (tinfo))
>+ {
>+ PyErr_Format (gdbpy_gdb_error, _("No such auxiliary object."));
>+ return NULL;
>+ }
>+
>+ if (btrace_find_insn_by_number (&iter, &tinfo->btrace, obj->number) == 0)
>+ {
>+ PyErr_Format (gdbpy_gdb_error, _("No such auxiliary object."));
>+ return NULL;
>+ }
>+
>+ insn = btrace_insn_get (&iter);
>+ if (insn == NULL)
>+ {
>+ PyErr_Format (gdbpy_gdb_error, _("Not a valid auxiliary object."));
>+ return NULL;
>+ }
>+
>+ return PyUnicode_FromString
>+ (iter.btinfo->aux_data.at (insn->aux_data_index).c_str ());

Shouldn't we check the insn class, too?

>@@ -455,10 +511,10 @@ btpy_list_item (PyObject *self, Py_ssize_t index)
>
> number = obj->first + (obj->step * index);
>
>- if (obj->element_type == &recpy_insn_type)
>- return recpy_insn_new (obj->thread, RECORD_METHOD_BTRACE, number);
>- else
>+ if (obj->element_type == &recpy_func_type)
> return recpy_func_new (obj->thread, RECORD_METHOD_BTRACE, number);
>+ else
>+ return btpy_item_new (obj->thread, number);
> }

Why do we need to change the order?

Regards,
Markus.