From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from paperclip.tbsaunde.org (tbsaunde.org [66.228.47.254]) by sourceware.org (Postfix) with ESMTP id 07221393C868 for ; Mon, 28 Jun 2021 07:01:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 07221393C868 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=tbsaunde.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tbsaunde.org Received: from rag (pool-108-24-42-131.cmdnnj.fios.verizon.net [108.24.42.131]) by paperclip.tbsaunde.org (Postfix) with ESMTPSA id 65E32C0DA; Mon, 28 Jun 2021 07:01:03 +0000 (UTC) Date: Mon, 28 Jun 2021 03:01:01 -0400 From: Trevor Saunders To: Martin Sebor Cc: Richard Biener , Richard Biener via Gcc-patches , Richard Sandiford Subject: Re: [PATCH 5/6] make get_domminated_by_region return a auto_vec Message-ID: References: <9a54c5e1-2d41-f499-e176-84913b810b67@gmail.com> <125764da-111e-7e44-b2ba-8389644423e8@gmail.com> <08396d8a-ff3e-ba6e-756a-ad4226b920a7@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-5.4 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 28 Jun 2021 07:01:06 -0000 On Wed, Jun 23, 2021 at 05:43:32PM -0600, Martin Sebor wrote: > On 6/22/21 11:23 PM, Trevor Saunders wrote: > > On Tue, Jun 22, 2021 at 02:01:24PM -0600, Martin Sebor wrote: > > > On 6/21/21 1:15 AM, Richard Biener wrote: > > > > On Fri, Jun 18, 2021 at 6:03 PM Martin Sebor wrote: > > > > > > > > > > On 6/18/21 4:38 AM, Richard Biener wrote: > > > > > > On Thu, Jun 17, 2021 at 4:43 PM Martin Sebor wrote: > > > > > > > > > > > > > > On 6/17/21 12:03 AM, Richard Biener wrote: > > > > > > > > On Wed, Jun 16, 2021 at 6:01 PM Martin Sebor wrote: > > > > > > > > > > > > > > > > > > On 6/16/21 6:46 AM, Richard Sandiford via Gcc-patches wrote: > > > > > > > > > > Richard Biener via Gcc-patches writes: > > > > > > > > > > > On Tue, Jun 15, 2021 at 8:02 AM Trevor Saunders wrote: > > > > > > > > > > > > > > > > > > > > > > > > This makes it clear the caller owns the vector, and ensures it is cleaned up. > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Trevor Saunders > > > > > > > > > > > > > > > > > > > > > > > > bootstrapped and regtested on x86_64-linux-gnu, ok? > > > > > > > > > > > > > > > > > > > > > > OK. > > > > > > > > > > > > > > > > > > > > > > Btw, are "standard API" returns places we can use 'auto'? That would avoid > > > > > > > > > > > excessive indent for > > > > > > > > > > > > > > > > > > > > > > - dom_bbs = get_dominated_by_region (CDI_DOMINATORS, > > > > > > > > > > > - bbs.address (), > > > > > > > > > > > - bbs.length ()); > > > > > > > > > > > + auto_vec dom_bbs = get_dominated_by_region (CDI_DOMINATORS, > > > > > > > > > > > + bbs.address (), > > > > > > > > > > > + bbs.length ()); > > > > > > > > > > > > > > > > > > > > > > and just uses > > > > > > > > > > > > > > > > > > > > > > auto dom_bbs = get_dominated_by_region (... > > > > > > > > > > > > > > > > > > > > > > Not asking you to do this, just a question for the audience. > > > > > > > > > > > > > > > > > > > > Personally I think this would be surprising for something that doesn't > > > > > > > > > > have copy semantics. (Not that I'm trying to reopen that debate here :-) > > > > > > > > > > FWIW, I agree not having copy semantics is probably the most practical > > > > > > > > > > way forward for now.) > > > > > > > > > > > > > > > > > > But you did open the door for me to reiterate my strong disagreement > > > > > > > > > with that. The best C++ practice going back to the early 1990's is > > > > > > > > > to make types safely copyable and assignable. It is the default for > > > > > > > > > all types, in both C++ and C, and so natural and expected. > > > > > > > > > > > > > > > > > > Preventing copying is appropriate in special and rare circumstances > > > > > > > > > (e.g, a mutex may not be copyable, or a file or iostream object may > > > > > > > > > not be because they represent a unique physical resource.) > > > > > > > > > > > > > > > > > > In the absence of such special circumstances preventing copying is > > > > > > > > > unexpected, and in the case of an essential building block such as > > > > > > > > > a container, makes the type difficult to use. > > > > > > > > > > > > > > > > > > The only argument for disabling copying that has been given is > > > > > > > > > that it could be surprising(*). But because all types are copyable > > > > > > > > > by default the "surprise" is usually when one can't be. > > > > > > > > > > > > > > > > > > I think Richi's "surprising" has to do with the fact that it lets > > > > > > > > > one inadvertently copy a large amount of data, thus leading to > > > > > > > > > an inefficiency. But by analogy, there are infinitely many ways > > > > > > > > > to end up with inefficient code (e.g., deep recursion, or heap > > > > > > > > > allocation in a loop), and they are not a reason to ban the coding > > > > > > > > > constructs that might lead to it. > > > > > > > > > > > > > > > > > > IIUC, Jason's comment about surprising effects was about implicit > > > > > > > > > conversion from auto_vec to vec. I share that concern, and agree > > > > > > > > > that it should be addressed by preventing the conversion (as Jason > > > > > > > > > suggested). > > > > > > > > > > > > > > > > But fact is that how vec<> and auto_vec<> are used today in GCC > > > > > > > > do not favor that. In fact your proposed vec<> would be quite radically > > > > > > > > different (and IMHO vec<> and auto_vec<> should be unified then to > > > > > > > > form your proposed new container). auto_vec<> at the moment simply > > > > > > > > maintains ownership like a smart pointer - which is _also_ not copyable. > > > > > > > > > > > > > > Yes, as we discussed in the review below, vec is not a good model > > > > > > > because (as you note again above) it's constrained by its legacy > > > > > > > uses. The best I think we can do for it is to make it safer to > > > > > > > use. > > > > > > > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571622.html > > > > > > > > > > > > Which is what Trevors patches do by simply disallowing things > > > > > > that do not work at the moment. > > > > > > > > > > > > > (Smart pointers don't rule out copying. A std::unique_ptr does > > > > > > > and std::shared_ptr doesn't. But vec and especially auto_vec > > > > > > > are designed to be containers, not "unique pointers" so any > > > > > > > relationship there is purely superficial and a distraction.) > > > > > > > > > > > > > > That auto_vec and vec share a name and an is-a relationship is > > > > > > > incidental, an implementation detail leaked into the API. A better > > > > > > > name than vector is hard to come up with, but the public inheritance > > > > > > > is a design flaw, a bug waiting to be introduced due to the conversion > > > > > > > and the assumptions the base vec makes about POD-ness and shallow > > > > > > > copying. Hindsight is always 20/20 but past mistakes should not > > > > > > > dictate the design of a general purpose vector-like container in > > > > > > > GCC. > > > > > > > > > > > > That auto_vec<> "decays" to vec<> was on purpose design. > > > > > > > > > > > > By-value passing of vec<> is also on purpose to avoid an extra > > > > > > pointer indirection on each access. > > > > > > > > > > I think you may have misunderstood what I mean by is-a relationship. > > > > > It's fine to convert an auto_vec to another interface. The danger > > > > > is in allowing that to happen implicitly because that tends to let > > > > > it happen even when it's not intended. The usual way to avoid > > > > > that risk is to provide a conversion function, like > > > > > auto_vec::to_vec(). This is also why standard classes like > > > > > std::vector or std::string don't allow such implicit conversions > > > > > and instead provide member functions (see for example Stroustrup: > > > > > The C++ Programming Language). So a safer auto_vec class would > > > > > not be publicly derived from vec but instead use the has-a design > > > > > (there are also ways to keep the derivation by deriving both from > > > > > from a limited, safe, interface, that each would extend as > > > > > appropriate). > > > > > > > > > > To the point of by passing vec by value while allowing functions > > > > > to modify the argument: C and C++ have by-value semantics. Every > > > > > C and C++ programmer knows and expect that. Designing interfaces > > > > > that break this assumption is perverse, a sure recipe for bugs. > > > > > If you're concerned about intuitive semantics and surprises you > > > > > should want to avoid that. > > > > > > > > > > > > > > > > > > I fully support fixing or at least mitigating the problems with > > > > > > > the vec base class (unsafe copying, pass-by-value etc.). As I > > > > > > > mentioned, I already started working on this cleanup. I also > > > > > > > have no objection to introducing a non-copyable form of a vector > > > > > > > template (I offered one in my patch), or even to making auto_vec > > > > > > > non-copyable provided a copyable and assignable one is introduced > > > > > > > at the same time, under some other name. > > > > > > > > > > > > Why at the same time? I'm still not convinced we need another > > > > > > vector type here. Yes, auto_vec > would be convenient, > > > > > > but then auto_vec<> doesn't bother to call the DTOR on its elements > > > > > > either (it's actually vec<> again here). So auto_vec<> is _not_ > > > > > > a fancier C++ vec<>, it's still just vec<> but with RAII for the container > > > > > > itself. > > > > > > > > > > I don't follow what you're saying. Either you agree that making > > > > > auto_vec suitable as its own element would be useful. If you do, > > > > > it needs to be safely copyable and assignable. > > > > > > > > > > The basic design principle of modern C++ containers is they store > > > > > their elements by value and make no further assumptions. This means > > > > > that an int element is treated the same as int* element as a vec > > > > > element: they're copied (or moved) by their ctors on insertion, > > > > > assigned when being replaced, and destroyed on removal. Containers > > > > > themselves don't, as a rule, manage the resources owned by > > > > > the elements (like auto_delete_vec does). The elements are > > > > > responsible for doing that, which is why they need to be safely > > > > > copyable and assignable. vec meets these requirements because > > > > > it doesn't manage a resource (it's not a container). Its memory > > > > > needs to be managed some other way. auto_vec doesn't. It is > > > > > designed to be a container but it's broken. It won't become one > > > > > by deleting its copy ctor and assignment. > > > > > > > > > > > > > > > > > > Having said that, and although I don't mind the cleanup being taken > > > > > > > off my plate, I would have expected the courtesy of at least a heads > > > > > > > up first. I do find it disrespectful for someone else involved in > > > > > > > the review of my work to at the same time submit a patch of their > > > > > > > own that goes in the opposite direction, and for you to unilaterally > > > > > > > approve it while the other review hasn't concluded yet. > > > > > > > > > > > > Because the changes do not change anything as far as I understand. > > > > > > They make more use of auto_vec<> ownership similar to when > > > > > > I added the move ctor and adjusted a single loop API. At the same > > > > > > time it completes the move stuff and plugs some holes. > > > > > > > > > > The vast majority of Trevor's changes are improvements and I apprciate > > > > > them. But the change to auto_vec goes against best C++ practices and > > > > > in the opposite direction of what I have been arguing for and what > > > > > I submitted a patch for in April. The patch is still under discussion > > > > > that both you and Trevor, as well as Jason, have been participating in. > > > > > We have not concluded that discussion and it's in bad form to simply > > > > > disregard that and proceed in a different direction. My understanding > > > > > from it so far is that > > > > > > > > > > a) you're not opposed to adding the copy ctor: > > > > > https://gcc.gnu.org/pipermail/gcc-patches/2021-April/568827.html > > > > > b) Jason advises to prevent implicit by-value conversion from auto_vec > > > > > to vec > > > > > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571628.html > > > > > c) I said I was working on it (and more, some of it likely now > > > > > obviated by Trevor's changes): > > > > > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571622.html > > > > > > > > > > So would I ask you both to respect that and refrain from approving > > > > > and committing this change (i.e., leave auto_vec as is for now) > > > > > until I've had time to finish my work. > > > > > > > > > > But checking the latest sources I see Trevor already committed > > > > > the change despite this. That's in very poor form, and quite > > > > > disrespectful of both of you. > > > > > > > > The change was needed to make the useful portions work as far > > > > as I understood Trevor. There's also nothing that prevents you > > > > from resolving the conflicts and continue with your improvements. > > > > > > The change disables things that were previously possible (but > > > undefined). It isn't relied on by anything. The change is > > > also incomplete: it disables copying and assignment for > > > the auto_vec specialization but not for the primary > > > template. Neither is safe to copy or assign. > > > > I suppose its technically true that it disables things that were > > possible, but for those things to work you'd have to jump through some > > hoops to prevent it causing a double free, so in practice I don't think > > you can really say it disables something, it just makes explicit what > > was already the case. Since the move constructors would have prevented > > the compiler from providing a default definition of the copy > > constructor, there deletion is essentially just documentation. Later > > patches in the series did depend on the move constructors to work > > properly. I would agree that the template with inline storage could use > > with adjustment in this area, but I think its fine to handle it > > separately per Richi's request, and further I suspect its much less > > useful and likely to cause trouble, the main use case would be for > > function location things, and returning such an object or passing it by > > value seems odd (not to say it is good the way it is, but that its of > > less practical importance). > > > > > I could still submit a patch to fix that and make all auto_vec > > > specializations safely copyable but why should I bother? You > > > have made it abundantly clear that you don't support it and > > > several others have taken your position despite all the problems > > > I have repeatedly pointed out. > > > > Well, personally if you wanted to define something like > > template T gcc::copy (const T &) = delete; WIth > > specializations for things that could be coppied including anything > > that's POD, I think that might be reasonable, and much clearer than "=" > > that calls malloc, if your issue is that .copy () on vec isn't the same > > as for other objects. Personally I think its mostly YAGNI and there's > > few enough places coppying things for it to be a real problem, but at > > the same time I think there's no particular need .copy () is a member, > > just that its use is more obvious to the author and reader than > > "=". > > auto_vec::copy() returns a vec so it's not the same thing as a copy > ctor/assignment. vec happens to be convertible to auto_vec so it's > possible to copy auto_vec A to auto_vec B by B = A.copy() but that's > a convoluted way of copying or assigning one object to another. Well, it should definitly return an auto_vec, but other than that I'm not sure what is so convoluted about it, other than preferences about the way it looks. > Without looking up the types of the operands, the meaning of > B = A.copy() is certainly less obvious than that of B = A. Same > for copy (B, A). To those of us accustomed to basic C++ idioms, > it's unusual to have to call a named function to do what's > canonically accomplished by a native operator. It raises > the question of what else the call might do that the expected Really? if a function's name is copy, what can it reasonably do other than produce a copy of its argument? > alternative wouldn't. To Richi's concern about the inefficiency > of deep copies, forcing users to call A.copy() to make one runs > the risk that they will do it even when it's not necessary and > when the move ctor or assignment would be suitable. It's much > better (and in line with the intended use of the feature) to > use the familiar initialization or assignment syntax and trust > the compiler make the right call whether to do a deep copy or > to move. I suppose its theoretically possible someone could call copy() and it not get questioned by a reviewer just like it is for any other function. However there is a function call there to make the reviewer remember to ask "wy?", rather than them needing to remember to ask at basically every use of "=" "should there be a std::move() here?". While it might be nice if the compiler could actually handle all this itself, it can't, and that isn't how the language works. First lets consider some examples. { auto_vec x; x.safe_push (1); auto_vec y (x); auto_vec z = y; } It should be obvious to a human that moves and no coppies would be fine here, however the compiler can't figure out that the objects are dead after the assignment, and so move candidates. Or for a more real world case of the same sort of thing I saw the other day: gimple_poly_bb (basic_block bb, auto_vec drs, auto_vec reads, auto_vec writes) : bb (bb), pbb (nullptr), data_refs (drs), read_scalar_refs (reads), write_scalar_refs (writes) {} Where its also completely clear the intention is to move the vectors, but the compiler doesn't figure that out, and so I'd say its very useful for people who aren't being watchful at every moment, that this simply fails to compile, rather than generating horribly inefficient code. Of course that is before we even start talking about cases where one needs to understand the algorithm or think about more than one scope at a time to realize the copy can be avoided, possibly by reorganizing other code. > More generally, obviating the distinction between native types > and user-defined types is one of the most powerful features of > C++. It makes it possible to write generic code: not just > templates but any piece of code that's agnostic of the types > it works with. There are many benefits: code that uses common > idioms and operators is easier to understand, swapping one data > type for another becomes trivial, and similarities between > different pieces of code are more apparent, opening up > opportunities for reuse and refactoring. Designing classes > to be gratuitously different means missing out on some of > the most valuable benefits of using the language. But I'm > just paraphrasing arguments that have been made much more > convincingly by authors of the many excellent C++ books out > there (some of my favorites are Herb Sutter's Exceptional C++ > series). Its certainly powerful, but with great power comes great responsibility, and its certainly a double edged sword, that should be used with care. While its certainly theoretically nice for everything to be uniform, and it sometimes has benefits, an int and a hash table with millions of elements are pretty different things and different operations make sense for them. Coppying one is pretty harmless, the other should involve a serious search for alternatives, if performance is a concern. So while I can understand that legacy constrains the standard library to just have one copy operation for everything, especially after C++11 I think that is a mistake that should be avoided in the future. Trev > > Martin