Re: [PATCH 6/7] Move language stuff out of varobj.c

Joel Brobecker
2013-10-17T05:40:26Z

Sorry for the delay in reviewing this...

> The new patch 4/7 affects this patch. I post the updated one. Patch
> 7/7 is not affected, so it can be reviewed too.
>
> --
> Yao (é½å°§)
>
> gdb:
>
> 2013-10-11 Yao Qi <yao@codesourcery.com>
>
> * Makefile.in (SFILES): Add c-varobj.c and jv-varobj.c.
> (COMMON_OBS): Add c-varobj.o and jv-varobj.o.
> * ada-varobj.c: Include "varobj.h".
> (ada_number_of_children): New. Moved from varobj.c.
> (ada_name_of_variable, ada_name_of_child): Likewise.
> (ada_path_expr_of_child, ada_value_of_child): Likewise.
> (ada_type_of_child, ada_value_of_variable): Likewise.
> (ada_value_is_changeable_p, ada_value_has_mutated): Likewise.
> (ada_varobj_ops): New.
> * c-varobj.c, jv-varobj.c: New file. Moved from varobj.c.
> * gdbtypes.c (get_target_type): New. Moved from varobj.c.
> * gdbtypes.h (get_target_type): Declare.
> * varobj.c: Remove the inclusion of "ada-varobj.h" and
> "ada-lang.h".
> (ANONYMOUS_STRUCT_NAME): Move it to c-varobj.c.
> (ANONYMOUS_UNION_NAME): Likewise.
> (get_type, get_value_type, get_target_type): Remove declarations.
> (value_get_print_value, varobj_value_get_print_value): Likewise.
> (c_number_of_children, c_name_of_variable): Likewise.
> (c_name_of_child, c_path_expr_of_child): Likewise.
> (c_value_of_child, c_type_of_child): Likewise.
> (c_value_of_variable, cplus_number_of_children): Likewise.
> (cplus_class_num_children, cplus_name_of_variable): Likewise.
> (cplus_name_of_child, cplus_path_expr_of_child): Likewise.
> (cplus_value_of_child, cplus_type_of_child): Likewise.
> (cplus_value_of_variable, java_number_of_children): Likewise.
> (java_name_of_variable, java_name_of_child): Likewise.
> (java_path_expr_of_child, java_value_of_child): Likewise.
> (java_type_of_child, java_value_of_variable): Likewise.
> (ada_number_of_children, ada_name_of_variable): Likewise.
> (ada_name_of_child, ada_path_expr_of_child): Likewise.
> (ada_value_of_child, ada_type_of_child): Likewise.
> (ada_value_of_variable, ada_value_is_changeable_p): Likewise.
> (ada_value_has_mutated): Likewise.
> (struct language_specific): Move it to varobj.h.
> (CPLUS_FAKE_CHILD): Move it to varobj.h.
> (restrict_range): Rename it varobj_restrict_range. Make it extern.
> Callers update.
> (get_path_expr_parent): Rename it to varobj_get_path_expr_parent.
> Make it extern.
> (is_anonymous_child): Move it to c-varobj.c and rename to
> varobj_is_anonymous_child. Caller update.
> (get_type): Move it to c-varobj.c.
> (get_value_type): Rename it varobj_get_value_type. Make it
> extern.
> (get_target_type): Move it gdbtypes.c.
> (varobj_formatted_print_options): New function.
> (value_get_print_value): Rename it to
> varobj_value_get_print_value and make it extern.
> (varobj_value_is_changeable_p): Make it extern.
> (adjust_value_for_child_access): Move it to c-varobj.c.
> (default_value_is_changeable_p): Rename it to
> varobj_default_value_is_changeable_p. Make it extern.
> (c_number_of_children, c_name_of_variable): Move it to c-varobj.c
> (c_name_of_child, c_path_expr_of_child): Likewise.
> (c_value_of_child, c_type_of_child): Likewise.
> (c_value_of_variable, cplus_number_of_children): Likewise.
> (cplus_class_num_children, cplus_name_of_variable): Likewise.
> (cplus_name_of_child, cplus_path_expr_of_child): Likewise.
> (cplus_value_of_child, cplus_type_of_child): Likewise.
> (cplus_value_of_variable): Likewise.
> (java_number_of_children, java_name_of_variable): Move it to jv-varobj.c.
> (java_name_of_child, java_path_expr_of_child): Likewise.
> (java_value_of_child, java_type_of_child): Likewise.
> (java_value_of_variable): Likewise.
> (ada_number_of_children, ada_name_of_variable): Move it to ada-varobj.c.
> (ada_name_of_child, ada_path_expr_of_child): Likewise.
> (ada_value_of_child, ada_type_of_child): Likewise.
> (ada_value_of_variable, ada_value_is_changeable_p): Likewise.
> (ada_value_has_mutated): Likewise.
> * varobj.h (CPLUS_FAKE_CHILD): New macro, moved from varobj.c.
> (struct lang_varobj_ops): New. Renamed by 'struct language_specific'.
> (c_varobj_ops, cplus_varobj_ops): Declare.
> (java_varobj_ops, ada_varobj_ops): Declare.
> (varobj_default_value_is_changeable_p): Declare.
> (varobj_value_is_changeable_p): Declare.
> (varobj_get_value_type, varobj_is_anonymous_child): Declare.
> (varobj_get_path_expr_parent): Declare.
> (varobj_value_get_print_value): Declare.
> (varobj_formatted_print_options): Declare.
> (varobj_restrict_range): Declare.

Overall, this looks pretty good to me. Just a few nits.

> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index 6c809a4..4da6ca2 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -239,6 +239,22 @@ get_type_arch (const struct type *type)
> return TYPE_OWNER (type).gdbarch;
> }
>
> +/* This returns the target type (or NULL) of TYPE, also skipping
> + past typedefs. */
> +
> +struct type *
> +get_target_type (struct type *type)

I noticed you documented this function twice, once in the header,
and ones in the body. Can you you only document it once? You have
two options: Remove the documentation in the header entirely, or
replace the documentation in the body by "See gdbtype.h" or some
such. I can't remember exactly the style that TomT introduced for
that - perhaps seem minsyms.h, IIRC.

> - /* Language info for this variable and its children. */
> - struct language_specific *lang;
> + /* Language-related operations for this variable and its
> + children. */
> + struct lang_varobj_ops *lang;

Please make that field a "const". That way...

> @@ -632,7 +434,7 @@ varobj_create (char *objname,
>
> /* Set language info */
> lang = variable_language (var);
> - var->root->lang = &languages[lang];
> + var->root->lang = (struct lang_varobj_ops *) languages[lang];

... we can get rid of the cast here.

Approved with those changes.

Thanks,
--
Joel