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