From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 36030 invoked by alias); 26 Oct 2017 16:37:42 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 36017 invoked by uid 89); 26 Oct 2017 16:37:41 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=no version=3.3.2 spammy=practices, confess X-HELO: mail-oi0-f47.google.com Received: from mail-oi0-f47.google.com (HELO mail-oi0-f47.google.com) (209.85.218.47) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 26 Oct 2017 16:37:39 +0000 Received: by mail-oi0-f47.google.com with SMTP id v9so6625966oif.13 for ; Thu, 26 Oct 2017 09:37:39 -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:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=uh1K5/AolI5vpaTe261UzXjTVyjNASvESDpugtb8otk=; b=pMak5rrF075cfrvO+cyWAgooSN5XiyWqB8t+ZiUfAqZ8mUwdoH5MgwDpiCYe5boFFf Z71sKdSfyPOZmCkkzztL26jAjzF/5BcUhKM+n4ODNq7RCwNKO4j6VRhim1/xgBGj4Tnw jeytOcmty01mGEpWD7gpljQwZa0SNJS4ER0o5tSHnUAIyFOWOsQV/v5wMsugyX0jz5ct 6kJx+m7Io8n3YYi6VnYZXmN/ubAvuVqYKT/HVXj7uwm2Hwl+wjnjsvPzfjd0e9b6fwiF 7rfFs209iNLdGysyn7AnKEjKh47qrvWsDCbJim0A4uKoEP2s8lo1HH02LeN88/JLymbv +RJQ== X-Gm-Message-State: AMCzsaWkaWKiOk7lOtvsYQ7xds6XdhSuquQBKA2MqWqgWddIaAGARc9e QMtO8WFnJNPXuGWcbjNbdDs= X-Google-Smtp-Source: ABhQp+RtdOyeJ2nJUdx86ac5Tk29hKj/u2o8+k7h/Kk7GdeeIJOFJchWmSKMBbhFhx9e0+eUnlmWzg== X-Received: by 10.202.83.79 with SMTP id h76mr2740726oib.310.1509035857436; Thu, 26 Oct 2017 09:37:37 -0700 (PDT) Received: from localhost.localdomain (71-218-5-6.hlrn.qwest.net. [71.218.5.6]) by smtp.gmail.com with ESMTPSA id j7sm2800411otd.68.2017.10.26.09.37.36 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 26 Oct 2017 09:37:36 -0700 (PDT) Subject: Re: [006/nnn] poly_int: tree constants To: gcc-patches@gcc.gnu.org, richard.sandiford@linaro.org References: <871sltvm7r.fsf@linaro.org> <878tg1u7cl.fsf@linaro.org> <877evi3oeh.fsf@linaro.org> <8737662tlp.fsf@linaro.org> From: Martin Sebor Message-ID: <2840a6a6-f80c-ca8f-aab2-2729ac598bf6@gmail.com> Date: Thu, 26 Oct 2017 16:45:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <8737662tlp.fsf@linaro.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2017-10/txt/msg01967.txt.bz2 >>>>> /* The tree and const_tree overload templates. */ >>>>> namespace wi >>>>> { >>>>> + class unextended_tree >>>>> + { >>>>> + private: >>>>> + const_tree m_t; >>>>> + >>>>> + public: >>>>> + unextended_tree () {} >>>> >>>> Defining no-op ctors is quite dangerous and error-prone. I suggest >>>> to instead default initialize the member(s): >>>> >>>> unextended_tree (): m_t () {} >>>> >>>> Ditto everywhere else, such as in: >>> >>> This is really performance-senesitive code though, so I don't think >>> we want to add any unnecessary initialisation. Primitive types are >>> uninitalised by default too, and the point of this class is to >>> provide an integer-like interface. >> >> I understand the performance concern (more on that below), but >> to clarify the usability issues, I don't think the analogy with >> primitive types is quite fitting here: int() evaluates to zero, >> as do the values of i and a[0] and a[1] after an object of type >> S is constructed using its default ctor, i.e., S (): >> >> struct S { >> int i; >> int a[2]; >> >> S (): i (), a () { } >> }; > > Sure, I realise that. I meant that: > > int x; > > doesn't initialise x to zero. So it's a question of which case is the > most motivating one: using "x ()" to initialise x to 0 in a constructor > or "int x;" to declare a variable of type x, uninitialised. I think the > latter use case is much more common (at least in GCC). Rearranging > things, I said later: I agree that the latter use case is more common in GCC, but I don't see it as a good thing. GCC was written in C and most code still uses now outdated C practices such as declaring variables at the top of a (often long) function, and usually without initializing them. It's been established that it's far better to declare variables with the smallest scope, and to initialize them on declaration. Compilers are smart enough these days to eliminate redundant initialization or assignments. >>> In your other message you used the example of explicit default >>> initialisation, such as: >>> >>> class foo >>> { >>> foo () : x () {} >>> unextended_tree x; >>> }; >>> >>> But I think we should strongly discourage that kind of thing. >>> If someone wants to initialise x to a particular value, like >>> integer_zero_node, then it would be better to do it explicitly. >>> If they don't care what the initial value is, then for these >>> integer-mimicing classes, uninitialised is as good as anything >>> else. :-) > > What I meant was: if you want to initialise "i" to 1 in your example, > you'd have to write "i (1)". Being able to write "i ()" instead of > "i (0)" saves one character but I don't think it adds much clarity. > Explicitly initialising something only seems worthwhile if you say > what you're initialising it to. My comment is not motivated by convenience. What I'm concerned about is that defining a default ctor to be a no-op defeats the zero-initialization semantics most users expect of T(). This is particularly concerning for a class designed to behave like an [improved] basic integer type. Such a class should act as closely as possible to the type it emulates and in the least surprising ways. Any sort of a deviation that replaces well- defined behavior with undefined is a gotcha and a bug waiting to happen. It's also a concern in generic (template) contexts where T() is expected to zero-initialize. A template designed to work with a fundamental integer type should also work with a user-defined type designed to behave like an integer. >> With the new (and some existing) classes that's not so, and it >> makes them harder and more error-prone to use (I just recently >> learned this the hard way about offset_int and the debugging >> experience is still fresh in my memory). > > Sorry about the bad experience. But that kind of thing cuts > both ways. If I write: > > poly_int64 > foo (void) > { > poly_int64 x; > x += 2; > return x; > } > > then I get a warning about x being used uninitialised, without > having had to run anything. If we add default initialisation > then this becomes something that has to be debugged against > a particular test case, i.e. we've stopped the compiler from > giving us useful static analysis. With default initialization the code above becomes valid and has the expected effect of adding 2 to zero. It's just more robust than the same code with that uses a basic type instead. This seems no more unexpected and no less desirable than the well- defined semantics of something like: std::string x; x += "2"; return x; or using any other C++ standard library type in a similar way. (Incidentally, although I haven't tried with poly_int, I get no warnings for the code above with offset_int or wide_int.) >> When the cor is inline and the initialization unnecessary then >> GCC will in most instances eliminate it, so I also don't think >> the suggested change would have a significant impact on >> the efficiency of optimized code, but... >> >> ...if it is thought essential to provide a no-op ctor, I would >> suggest to consider making its property explicit, e.g., like so: >> >> struct unextended_tree { >> >> struct Uninit { }; >> >> // ... >> unextended_tree (Uninit) { /* no initialization */ } >> // ... >> }; >> >> This way the programmer has to explicitly opt in to using the >> unsafe ctor. (This ctor is suitable for single objects, not >> arrays of such things, but presumably that would be sufficient. >> If not, there are tricks to make that work too.) > > The default constructors for unextended_tree and extended_tree > are only there for the array case (in poly-int.h). My main concern is with the new poly_int classes (and the existing wide int classes) because I think those are or will be widely used, far more so than the unextended_tree class (I confess this review is the first time I've ever noticed it). > Part of the problem here is that we still have to live by C++03 > POD rules. If we moved to C++11, the need for the poly_int_pod/ > poly_int split would go away and things would probably be much > simpler. :-) Understood. With the heavy use of templates, template templates, and partial specialization, the poly_int classes will push older C++ 98 compilers to their limits. It seems that for stability's sake it would make sense to require a more modern compiler. >>> Note that for this class NULL_TREE is not a safe default value. >>> The same goes for the wide-int.h classes, where a length or precision >>> of 0 is undefined and isn't necessarily going to be handled gracefully >>> or predictably. >> >> For offset_int both precision and length are known so I think >> it would make sense to have the default ctor value-initialize >> the object. For wide_int, it seems to me that choosing some >> default precision and length in the default ctor would still >> be preferable to leaving the members indeterminate. (That >> functionality could still be provided by some other ctor as >> I suggested above). > > But which precision though? If we pick a commonly-used one > then we make a missing initialisation bug very data-dependent. > Even if we pick a rarely-used one, we create a bug in which > the wide_int has the wrong precision even though all assignments > to it "obviously" have the right precision. For offset_int the default precision is 128-bits. Making that the default also for wide_int should be unsurprising. >> I only meant to suggest that the comment be reworded so as >> not to imply that such pointers (that are farther apart than >> PTRDIFF_MAX) can point to the same object and be subtracted. > > OK, how about: > > /* Return true if T holds a polynomial pointer difference, storing it in > *VALUE if so. A true return means that T's precision is no greater > than 64 bits, which is the largest address space we support, so *VALUE > never loses precision. However, the signedness of the result does > not necessarily match the signedness of T: sometimes an unsigned type > like sizetype is used to encode a value that is actually negative. */ That looks good to me. Thanks Martin