[PATCH 5/6] make get_domminated_by_region return a auto_vec

[Richard Biener]

On Thu, Jun 24, 2021 at 12:56 AM Martin Sebor <msebor@gmail.com> wrote:
>
> On 6/23/21 1:43 AM, Richard Biener wrote:
> > On Wed, Jun 23, 2021 at 7:23 AM Trevor Saunders <tbsaunde@tbsaunde.org> 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?

Thanks,
Richard.

> Martin