From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25197 invoked by alias); 18 Dec 2013 03:44:24 -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 25180 invoked by uid 89); 18 Dec 2013 03:44:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.0 required=5.0 tests=AWL,BAYES_00 autolearn=ham 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; Wed, 18 Dec 2013 03:44:19 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 8699D11677B; Tue, 17 Dec 2013 22:44:57 -0500 (EST) 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 s8dVwZVKdNYu; Tue, 17 Dec 2013 22:44:57 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id EC541116767; Tue, 17 Dec 2013 22:44:56 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 6B553E1C3A; Wed, 18 Dec 2013 07:44:12 +0400 (RET) Date: Wed, 18 Dec 2013 03:44:00 -0000 From: Joel Brobecker To: Sanimir Agovic Cc: tromey@redhat.com, palves@redhat.com, xdje42@gmail.com, gdb-patches@sourceware.org, keven.boell@intel.com Subject: Re: [PATCH v4 05/13] vla: update type from newly created value Message-ID: <20131218034412.GE3493@adacore.com> References: <1387282678-3847-1-git-send-email-sanimir.agovic@intel.com> <1387282678-3847-6-git-send-email-sanimir.agovic@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1387282678-3847-6-git-send-email-sanimir.agovic@intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2013-12/txt/msg00684.txt.bz2 > Constructing a value based on a type and address might change the type > of the newly constructed value. Thus re-fetch type via value_type to ensure > we have the correct type at hand. > > 2013-10-18 Sanimir Agovic > Keven Boell > > * ada-lang.c (ada_value_primitive_packed_val): Re-fetch type from value. > (ada_template_to_fixed_record_type_1): Likewise. > (ada_to_fixed_type_1): Likewise. > * cp-valprint.c (cp_print_value_fields_rtti): Likewise. > (cp_print_value): Likewise. > * d-valprint.c (dynamic_array_type): Likewise. > * jv-valprint.c (java_value_print): Likewise. > * valops.c (value_ind): Likewise. > * value.c (coerce_ref): Likewise. This patch makes me a little nervous, but unfortunately, the only option I have to help with that is going to be a little labor-intensive, so may not be practical. I'll leave it to you and the maintainers who have been reviewing your patches so far. I see that the type re-fetch was not added systematically, but only in some of the locations where value_at/value_at_lazy/ value_from_contents_and_address are being called? Was it in order to fix some regressions revealed by the testsuite? If that's the case, I think this patch should be merged with the patch that causes the regressions, just to make sure that each patch individually causes no regression. This also helps when using the bisect feature of git. It looks like it's very easy to miss a case where we should re-fetch the type, which is what makes me slightly nervous. It's also putting the onus on the user to remember that value_[...] may return a value whose type is different from the one he has. So, now the labor-intensive suggestion: How about adding a type parameter to those 3 functions, to force the user to think about it and give him a chance to DTRT from the start? This parameter would be allowed to be NULL in the cases were we know we don't need to re-fecth. Another advantage I see from this approach is that vendor code-bases would stop building as soon as your change is imported, directing their attention to this change and the implicit question, for each location, that needs to be answered. I did a quick grep, and counted about 70 locations that would need to be adjusted if we were to go that route. -- Joel