From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3086 invoked by alias); 8 Oct 2010 22:54:56 -0000 Received: (qmail 3076 invoked by uid 22791); 8 Oct 2010 22:54:55 -0000 X-SWARE-Spam-Status: No, hits=-6.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 08 Oct 2010 22:54:50 +0000 Received: from int-mx08.intmail.prod.int.phx2.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o98MsiCl001843 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 8 Oct 2010 18:54:45 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx08.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o98MsjRw011795; Fri, 8 Oct 2010 18:54:45 -0400 Received: from opsy.redhat.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id o98MshYE019654; Fri, 8 Oct 2010 18:54:43 -0400 Received: by opsy.redhat.com (Postfix, from userid 500) id CE0473797CC; Fri, 8 Oct 2010 16:54:42 -0600 (MDT) From: Tom Tromey To: sami wagiaalla Cc: Eli Zaretskii , gdb-patches@sourceware.org Subject: Re: [patch 2/2] Fix overload resolution of int* vs void* References: <4C7BCD42.9070308@redhat.com> <4CAF6B73.5090800@redhat.com> <8339sgo1jl.fsf@gnu.org> <4CAF889C.1080404@redhat.com> Date: Fri, 08 Oct 2010 22:54:00 -0000 In-Reply-To: <4CAF889C.1080404@redhat.com> (sami wagiaalla's message of "Fri, 08 Oct 2010 17:09:48 -0400") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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 X-SW-Source: 2010-10/txt/msg00168.txt.bz2 >>>>> "Sami" == sami wagiaalla writes: Sami> @@ -1886,6 +1886,14 @@ do_is_ancestor (struct type *base, struct type *dclass, int public) Sami> if (class_types_same_p (base, dclass)) Sami> return 1; Sami> + if ((TYPE_CODE (base) == TYPE_CODE_PTR Sami> + && TYPE_CODE (dclass) == TYPE_CODE_PTR) Sami> + || (TYPE_CODE (base) == TYPE_CODE_REF Sami> + && TYPE_CODE (dclass) == TYPE_CODE_REF)) Sami> + return do_is_ancestor (TYPE_TARGET_TYPE (base), Sami> + TYPE_TARGET_TYPE (dclass), Sami> + public); This seems strange to me. It ought to only be applicable at the outermost call. But in that case, I think a wrapper function would be more appropriate, so that do_is_ancestor has a single purpose. Also, is this the right thing for the caller when there are multiple pointers (like "Class ***")? Or do you intend to only strip a single pointer? Sami> + /* Resolve typedefs */ Sami> + if (TYPE_CODE (a) == TYPE_CODE_TYPEDEF) Sami> + a = check_typedef (a); Sami> + if (TYPE_CODE (b) == TYPE_CODE_TYPEDEF) Sami> + b = check_typedef (b); It is ok to call check_typedef unconditionally, but I see this is just copied from rank_one_type. Sami> + /* (b) pointer to ancestor-pointer conversion. */ Sami> + if (is_public_ancestor (parm, arg)) Sami> + return BASE_PTR_CONVERSION_BADNESS; I am curious why you didn't just give POINTER_CONVERSION_BADNESS a new value and instead introduced BASE_PTR_CONVERSION_BADNESS. But then, I also don't understand the existing code that returns POINTER_CONVERSION_BADNESS... Also, why specifically is_public_ancestor and not is_ancestor? I suppose this is an incremental patch -- still not truly correct with regards to the distance-to-base problem, but an improvement over what gdb currently does. This is fine, even great, but it is handy for reviewers if you note that sort of thing in the patch explanation. Sami> case TYPE_CODE_ARRAY: Sami> - return rank_one_type (TYPE_TARGET_TYPE (parm), Sami> - TYPE_TARGET_TYPE (arg)); Sami> + if (types_equal (TYPE_TARGET_TYPE (parm), Sami> + TYPE_TARGET_TYPE (arg))) Sami> + return 0; Sami> + return INCOMPATIBLE_TYPE_BADNESS; Do the new tests cover this case? I could not immediately tell. Tom