Re: [PATCH] Fix py-xmethods.c when compiled with -Werror against Python 2.4

[Doug Evans <dje at google dot com>]

On Wed, Jun 4, 2014 at 11:13 AM, Joel Brobecker <brobecker@adacore.com> wrote:
>> Does the attached patch fix the issue pointed out by Ulrich Weigand
>> here: https://sourceware.org/ml/gdb-patches/2014-06/msg00169.html
>>
>> ChangeLog
>> 2014-06-04  Siva Chandra Reddy  <sivachandra@google.com>
>>
>>         * python/py-xmethods.c (invoke_match_method)
>>         (gdbpy_get_matching_xmethod_workers, gdbpy_get_xmethod_arg_types):
>>         Cast the second arg to PyObject_GetAttrString and
>>         PyObject_GetAttrString to char *.
>
> I can't tell whether it fixes the problem - it should! - but looking
> at the patch, I think some lines might have become longer than 80
> characters...
>
> Also, it'd be nice to answer Ulrich's question regarding the use
> of the constants - whether it might make sense to use the string
> directly? Looking at the code, I think that it would be to avoid
> duplicating that string? enabled_field_name is only used once,
> but then you'd probably use the constant for ... consistency (;-)).

Heh.  Except that this "consistency" exists to work around a problem
that is little documented.
[There's no comments in the code explaining why things are the way they are,
so it's completely not unexpected that someone might come along and think they
could just move those strings into const globals and everything would
be peachy.]

>
>> diff --git a/gdb/python/py-xmethods.c b/gdb/python/py-xmethods.c
>> index 0062b2d..5ba146f 100644
>> --- a/gdb/python/py-xmethods.c
>> +++ b/gdb/python/py-xmethods.c
>> @@ -106,7 +106,7 @@ invoke_match_method (PyObject *matcher, PyObject *py_obj_type,
>>
>>    cleanups = make_cleanup (null_cleanup, NULL);
>>
>> -  enabled_field = PyObject_GetAttrString (matcher, enabled_field_name);
>> +  enabled_field = PyObject_GetAttrString (matcher, (char *) enabled_field_name);
>>    if (enabled_field == NULL)
>>      {
>>        do_cleanups (cleanups);
>> @@ -127,7 +127,7 @@ invoke_match_method (PyObject *matcher, PyObject *py_obj_type,
>>        Py_RETURN_NONE;
>>      }
>>
>> -  match_method = PyObject_GetAttrString (matcher, match_method_name);
>> +  match_method = PyObject_GetAttrString (matcher, (char *) match_method_name);
>>    if (match_method == NULL)
>>      {
>>        do_cleanups (cleanups);
>> @@ -252,13 +252,13 @@ gdbpy_get_matching_xmethod_workers
>>
>>    /* Gather debug method matchers registered globally.  */
>>    if (gdb_python_module != NULL
>> -      && PyObject_HasAttrString (gdb_python_module, matchers_attr_str))
>> +      && PyObject_HasAttrString (gdb_python_module, (char *) matchers_attr_str))
>>      {
>>        PyObject *gdb_matchers;
>>        PyObject *temp = py_xmethod_matcher_list;
>>
>>        gdb_matchers = PyObject_GetAttrString (gdb_python_module,
>> -                                          matchers_attr_str);
>> +                                          (char *) matchers_attr_str);
>>        if (gdb_matchers != NULL)
>>       {
>>         py_xmethod_matcher_list = PySequence_Concat (temp, gdb_matchers);
>> @@ -391,8 +391,8 @@ gdbpy_get_xmethod_arg_types (const struct extension_language_defn *extlang,
>>
>>    cleanups = ensure_python_env (get_current_arch (), current_language);
>>
>> -  get_arg_types_method =  PyObject_GetAttrString (py_worker,
>> -                                               get_arg_types_method_name);
>> +  get_arg_types_method = PyObject_GetAttrString
>> +    (py_worker, (char *) get_arg_types_method_name);
>>    if (get_arg_types_method == NULL)
>>      {
>>        gdbpy_print_stack ();

The patch is fine to me, fwiw.
I wouldn't want to litter every instance of the code with comments
explaining the cast.
I think a good place to document the issue is gdb/python/README, fwiw.