[PATCH 5/6] make get_domminated_by_region return a auto_vec
Thu Jun 24 15:01:55 GMT 2021
On 6/24/21 3:27 AM, Richard Biener wrote:
> On Thu, Jun 24, 2021 at 12:56 AM Martin Sebor <firstname.lastname@example.org> wrote:
>> On 6/23/21 1:43 AM, Richard Biener wrote:
>>> On Wed, Jun 23, 2021 at 7:23 AM Trevor Saunders <email@example.com> wrote:
>>>> On Tue, Jun 22, 2021 at 02:01:24PM -0600, Martin Sebor wrote:
>>>>> On 6/21/21 1:15 AM, Richard Biener wrote:
>>>>>> But maybe I'm misunderstanding C++ too much :/
>>>>>> Well, I guess b) from above means auto_vec<> passing to
>>>>>> vec<> taking functions will need changes?
>>>>> Converting an auto_vec object to a vec slices off its data members.
>>>>> The auto_vec<T, 0> specialization has no data members so that's not
>>>>> a bug in and of itself, but auto_vec<T, N> does have data members
>>>>> so that would be a bug. The risk is not just passing it to
>>>>> functions by value but also returning it. That risk was made
>>>>> worse by the addition of the move ctor.
>>>> I would agree that the conversion from auto_vec<> to vec<> is
>>>> questionable, and should get some work at some point, perhaps just
>>>> passingauto_vec references is good enough, or perhaps there is value in
>>>> some const_vec view to avoid having to rely on optimizations, I'm not
>>>> sure without looking more at the usage.
>>> We do need to be able to provide APIs that work with both auto_vec<>
>>> and vec<>, I agree that those currently taking a vec<> by value are
>>> fragile (and we've had bugs there before), but I'm not ready to say
>>> that changing them all to [const] vec<>& is OK. The alternative
>>> would be passing a const_vec<> by value, passing that along to
>>> const vec<>& APIs should be valid then (I can see quite some API
>>> boundary cleanups being necessary here ...).
>>> But with all this I don't know how to adjust auto_vec<> to no
>>> longer "decay" to vec<> but still being able to pass it to vec<>&
>>> and being able to call vec<> member functions w/o jumping through
>>> hoops. Any hints on that? private inheritance achieves the first
>>> but also hides all the API ...
>> The simplest way to do that is by preventing the implicit conversion
>> between the two types and adding a to_vec() member function to
>> auto_vec as Jason suggested. This exposes the implicit conversions
>> to the base vec, forcing us to review each and "fix" it either by
>> changing the argument to either vec& or const vec&, or less often
>> to avoid the indirection if necessary, by converting the auto_vec
>> to vec explicitly by calling to_vec(). The attached diff shows
>> a very rough POC of what that might look like. A side benefit
>> is that it improves const-safety and makes the effects of functions
>> on their callers easier to understand.
>> But that's orthogonal to making auto_vec copy constructible and copy
>> assignable. That change can be made independently and with much less
>> effort and no risk.
> There's a bunch of stuff I can't review because of lack of C++ knowledge
> (the vNULL changes).
> I suppose that
> + /* Prevent implicit conversion from auto_vec. Use auto_vec::to_vec()
> + instead. */
> + template <size_t N>
> + vec (auto_vec<T, N> &) = delete;
> + template <size_t N>
> + void operator= (auto_vec<T, N> &) = delete;
> still allows passing auto_vec<> to [const] vec<> & without the .to_vec so
> it's just the by-value passing that becomes explicit? If so then I agree
> this is an improvement. The patch is likely incomplete though or is
> it really only so few signatures that need changing?
Yes, to both questions.
I just wanted to show how we might go about making this transition.
I converted a few files but many more remain. I stopped when
changing a vec argument to const vec& started to cause errors due
to missing const down the line (e.g., assigning the address of a vec
element to an uqualified pointer). I'll need to follow where that
pointer flows and see if it's used to modify the object or if it too
could be made const. To me that seems worthwhile doing now but it
makes progress slow.
A separate question is whether the vec value arguments to functions
that modify them should be changed to references or pointers. Both
kinds of APIs exist but the latter seems prevalent. Changing them
to the latter means more churn.
More information about the Gcc-patches