From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30542 invoked by alias); 28 Apr 2017 09:51:02 -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 30490 invoked by uid 89); 28 Apr 2017 09:51:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_NONE autolearn=no version=3.3.2 spammy=totally, blown, suspect, they're X-HELO: mail-wm0-f47.google.com Received: from mail-wm0-f47.google.com (HELO mail-wm0-f47.google.com) (74.125.82.47) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 28 Apr 2017 09:50:58 +0000 Received: by mail-wm0-f47.google.com with SMTP id r190so41791439wme.1 for ; Fri, 28 Apr 2017 02:51:00 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=yJM90KXSULGpt1pkMd1aRPWl/JodJKzVYihCn3PjDyw=; b=oBJngxpS7uwOneFvpM6/Z44MCZjJssy3XLzgRNHhEyv4efgjD+1ChZtHzC4BK+aWrG rffUvPSgKaJntJ24hmfF+kZ7k7rCGO77X1ynK9ovCI2sx8VMfgnqo5hUKO7qIFhIJts2 VgLH/6koDUBygYoCTa1WqED3K9K+Lml3hDCkilpsvmwDtyfARqWxGTLzOD6fKN+62CJg zuIfSAAUzrkn9opOd8TL0pSS+mj2fEJOyQE9Tk1V6OlNy3M2JLWpWcdsCicv8UzCBLS4 Nw30rW9DSv6vkeO4euFckTqYrTsxWjK0vUSdf0/6PK+LWfv4cJ+HWKrvO9ISC2v2+M8A haIQ== X-Gm-Message-State: AN3rC/76ryVzYG4LjrRDwefQwme9AB69WdII/vwlJVUDLUnACjwkzzJ8 XOfD4fr0TJeqQo71 X-Received: by 10.28.198.65 with SMTP id w62mr5030099wmf.80.1493373058779; Fri, 28 Apr 2017 02:50:58 -0700 (PDT) Received: from [192.168.0.102] ([37.189.166.198]) by smtp.gmail.com with ESMTPSA id v80sm5046342wmv.4.2017.04.28.02.50.57 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 28 Apr 2017 02:50:57 -0700 (PDT) Subject: Re: [PATCH 5/6] Use copy ctor in regcache_dup To: Yao Qi References: <1493152106-3246-1-git-send-email-yao.qi@linaro.org> <1493152106-3246-6-git-send-email-yao.qi@linaro.org> <86shksc2xb.fsf@gmail.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: Date: Fri, 28 Apr 2017 09:51:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <86shksc2xb.fsf@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-04/txt/msg00767.txt.bz2 On 04/28/2017 10:11 AM, Yao Qi wrote: > Pedro Alves writes: > >> This one doesn't look right to me. This isn't a copy in the >> normal C++ object copy sense. The new object isn't semantically the >> same as the source. One can't use the new object the same way as the >> source regcache, they're not interchangeable. This is bound to generate >> confusion and problems. > > I thought about this. The reason I still do this is that I can't think > of a case that we need to copy a read-write regcache to another > read-write regcache. So far, we only use copy(or transform) a > read-write regcache to a read-only regcache. However, I agree with you, > it is not a normal "copy ctor". > >> >> Considering patch #6, it'd make more sense to me to >> make that a separate constructor with tag dispatching, like: >> >> struct regcache >> { >> struct readonly_t {}; >> static constexpr readonly_t readonly {}; >> >> regcache (readonly_t, const regcache &src); // old regcache_dup >> }; >> >> Then used like: >> >> regcache ro_copy (regcache::readonly, src); >> >> or if you want, you could make that tag-based ctor private and >> add a factory function: >> > > I have a different design on this, that is, put readonly regcache and > readwrite regcache to two classes. readwrite regcache inherits readonly > regcache, and readonly regcache has a constructor whose argument is a > readwrite regcache. *nod* I had seen it in the series' intro, but was going with the idea of avoid that work. If we can get it nicely expressed in the type system, then it's even better. > > class readonly_regcache > { > public: > explicit readonly_regcache (const regcache &); > } > > class regcache : public readonly_regcache > { > } > > What do you think? I think that that's too incomplete to evaluate. :-) Why regcache on top of readonly_regcache and not the other way around? Off hand, I'd think that struct regcache_base; struct readonly_regcache : regcache_base {}; struct regcache : regcache_base {}; would be the "obvious" first choice. What happens to all the "regcache->readonly_p" checks in spread around in multiple functions? Do they disappear? I think that if the design ends up with that flag still present and functions exposing interfaces that work with a "struct regcache *" that can either be readonly or write-through, then it's likely that the design isn't complete. [But replacing a single boolean checked in a few select places by virtual methods and a full blown vtable and a bunch of dispatching makes me cringe a bit too. :-) But OTOH, I suspect you want to add virtual methods for unit testing.] I'm totally not against this direction, to be clear, but it'd still suggest adding the tag dispatch ctor first (the simple version with no factory, just adds two lines of code compared to the copy ctor version), which allows getting rid of the heap allocation and the cleanups as you're doing in patch #6, and then consider changing the hierarchy in a follow up patch. > >> >> In any case, I think we should make sure to disable >> the regular copy methods since the type doesn't really >> support normal copy: >> >> regcache(const regcache &) = delete; >> void operator= (const regcache &) = delete; > > I agree. I'll add it. Thanks. -- Pedro Alves