From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26869 invoked by alias); 14 Apr 2016 13:45:46 -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 26642 invoked by uid 89); 14 Apr 2016 13:45:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_NONE autolearn=no version=3.3.2 spammy=wondering 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, 14 Apr 2016 13:45:42 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 5A923116B23; Thu, 14 Apr 2016 09:45:40 -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 c2qJOLe4AWbq; Thu, 14 Apr 2016 09:45:40 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 21C04116B21; Thu, 14 Apr 2016 09:45:39 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 75F6840780; Thu, 14 Apr 2016 06:45:38 -0700 (PDT) Date: Thu, 14 Apr 2016 13:45:00 -0000 From: Joel Brobecker To: Bernhard Heckel Cc: yao@codesourcery.com, gdb-patches@sourceware.org Subject: Re: [PATCH V2 1/3] fort_dyn_array: Enable dynamic member types inside a structure. Message-ID: <20160414134538.GA5026@adacore.com> References: <1459936659-19039-1-git-send-email-bernhard.heckel@intel.com> <1459936659-19039-2-git-send-email-bernhard.heckel@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1459936659-19039-2-git-send-email-bernhard.heckel@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-SW-Source: 2016-04/txt/msg00331.txt.bz2 Just a few minor nits that I happen to see while glancing at the patch: > --- a/gdb/gdbtypes.c > +++ b/gdb/gdbtypes.c > @@ -2064,7 +2064,8 @@ resolve_dynamic_struct (struct type *type, > > pinfo.type = check_typedef (TYPE_FIELD_TYPE (type, i)); > pinfo.valaddr = addr_stack->valaddr; > - pinfo.addr = addr_stack->addr; > + pinfo.addr = addr_stack->addr > + + (TYPE_FIELD_BITPOS (resolved_type, i) / TARGET_CHAR_BIT); The GCS recommends in that case to use an extra pair of parens. The purpose is to help code formatters align the code properly. pinfo.addr = (addr_stack->addr + (TYPE_FIELD_BITPOS (resolved_type, i) / TARGET_CHAR_BIT)); > + /* The length of a type won't change for fortran, but it does for C and Ada. > + For fortran the size of dynamic fields might change over time but not the > + type length of the structure. If we would adapt it we run into problems > + when calculating the element offset for arrays of structs. */ May I suggest the following rephrasing, which sounds a litle better to me? "If we adapt it, we run into problems [...]" > +/* Remove dynamic property from TYPE in case it exist. */ "exists" > +void > +remove_dyn_prop (enum dynamic_prop_node_kind prop_kind, > + struct type *type) > +{ > + struct dynamic_prop_list *prev_node, *curr_node; > + > + curr_node = TYPE_DYN_PROP_LIST (type); > + prev_node = NULL; > + > + while (NULL != curr_node) > + { > + if (curr_node->prop_kind == prop_kind) > + { > + /* Upadate the linked list but don't free anything. "Update" > + /* Internal variables which are created from values with a dynamic location > + don't need the location property of the origin anymore. > + Remove the location property in case it exist. */ > + remove_dyn_prop (DYN_PROP_DATA_LOCATION, value_type (new_data.value)); I am also wondering why it makes a difference to remove it, and it would be useful to explain it here. -- Joel