From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3985 invoked by alias); 17 Oct 2013 05:40:26 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 3968 invoked by uid 89); 17 Oct 2013 05:40:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=1.3 required=5.0 tests=AWL,BAYES_00,GARBLED_BODY autolearn=no version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Thu, 17 Oct 2013 05:40:24 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 84F5811669E; Thu, 17 Oct 2013 01:40:44 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id pxzpBQF-ea4k; Thu, 17 Oct 2013 01:40:44 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 1EB1D116696; Thu, 17 Oct 2013 01:40:43 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 4B256E10A0; Thu, 17 Oct 2013 09:40:17 +0400 (RET) Date: Thu, 17 Oct 2013 05:40:00 -0000 From: Joel Brobecker To: Yao Qi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 6/7] Move language stuff out of varobj.c Message-ID: <20131017054017.GA3302@adacore.com> References: <1379512482-31773-1-git-send-email-yao@codesourcery.com> <1379512482-31773-7-git-send-email-yao@codesourcery.com> <5257B481.9030003@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <5257B481.9030003@codesourcery.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2013-10/txt/msg00506.txt.bz2 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 > > * 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