On 6/2/21 12:55 AM, Richard Biener wrote:
On Tue, Jun 1, 2021 at 9:56 PM Martin Sebor <mse...@gmail.com> wrote:
On 5/27/21 2:53 PM, Jason Merrill wrote:
On 4/27/21 11:52 AM, Martin Sebor via Gcc-patches wrote:
On 4/27/21 8:04 AM, Richard Biener wrote:
On Tue, Apr 27, 2021 at 3:59 PM Martin Sebor <mse...@gmail.com> wrote:
On 4/27/21 1:58 AM, Richard Biener wrote:
On Tue, Apr 27, 2021 at 2:46 AM Martin Sebor via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
PR 90904 notes that auto_vec is unsafe to copy and assign because
the class manages its own memory but doesn't define (or delete)
either special function. Since I first ran into the problem,
auto_vec has grown a move ctor and move assignment from
a dynamically-allocated vec but still no copy ctor or copy
assignment operator.
The attached patch adds the two special functions to auto_vec along
with a few simple tests. It makes auto_vec safe to use in containers
that expect copyable and assignable element types and passes
bootstrap
and regression testing on x86_64-linux.
The question is whether we want such uses to appear since those
can be quite inefficient? Thus the option is to delete those
operators?
I would strongly prefer the generic vector class to have the properties
expected of any other generic container: copyable and assignable. If
we also want another vector type with this restriction I suggest to add
another "noncopyable" type and make that property explicit in its name.
I can submit one in a followup patch if you think we need one.
I'm not sure (and not strictly against the copy and assign). Looking
around
I see that vec<> does not do deep copying. Making auto_vec<> do it
might be surprising (I added the move capability to match how vec<>
is used - as "reference" to a vector)
The vec base classes are special: they have no ctors at all (because
of their use in unions). That's something we might have to live with
but it's not a model to follow in ordinary containers.
I don't think we have to live with it anymore, now that we're writing
C++11.
The auto_vec class was introduced to fill the need for a conventional
sequence container with a ctor and dtor. The missing copy ctor and
assignment operators were an oversight, not a deliberate feature.
This change fixes that oversight.
The revised patch also adds a copy ctor/assignment to the auto_vec
primary template (that's also missing it). In addition, it adds
a new class called auto_vec_ncopy that disables copying and
assignment as you prefer.
Hmm, adding another class doesn't really help with the confusion richi
mentions. And many uses of auto_vec will pass them as vec, which will
still do a shallow copy. I think it's probably better to disable the
copy special members for auto_vec until we fix vec<>.
There are at least a couple of problems that get in the way of fixing
all of vec to act like a well-behaved C++ container:
1) The embedded vec has a trailing "flexible" array member with its
instances having different size. They're initialized by memset and
copied by memcpy. The class can't have copy ctors or assignments
but it should disable/delete them instead.
2) The heap-based vec is used throughout GCC with the assumption of
shallow copy semantics (not just as function arguments but also as
members of other such POD classes). This can be changed by providing
copy and move ctors and assignment operators for it, and also for
some of the classes in which it's a member and that are used with
the same assumption.
3) The heap-based vec::block_remove() assumes its elements are PODs.
That breaks in VEC_ORDERED_REMOVE_IF (used in gcc/dwarf2cfi.c:2862
and tree-vect-patterns.c).
I took a stab at both and while (1) is easy, (2) is shaping up to
be a big and tricky project. Tricky because it involves using
std::move in places where what's moved is subsequently still used.
I can keep plugging away at it but it won't change the fact that
the embedded and heap-based vecs have different requirements.
So you figured that neither vec<> nor auto_vec<> are a container like
std::vector.
That's obvious from glancing at their definitions. I didn't go
through the exercise to figure that out.
I'm not sure it makes sense to try to make it so since obviously vec<>
was designed to match the actual needs of GCC. auto_vec<> was added
to make a RAII (like auto_bitmap, etc.) wrapper, plus it got the ability
to provide initial stack storage.
The goal was to see if the two vec instances could be made safer
to use but taking advantage of C++ 11 features. As I mentioned
recently, creating a copy of a vec and modifying it changes it as
well as the original (e.g., by changing a vec argument passed to
it by value a function changes the actual argument in the caller).
That's surprising to most C++ programmers.
My conclusion from the exercise is that although some of the problems
with vec can, and IMO should, be solved, making the heap-based one
a well-behaved C++ 11 container will take considerable effort and
is impossible for the embedded vec.
It doesn't seem to me that having a safely copyable auto_vec needs
to be put on hold until the rats nest above is untangled. It won't
make anything worse than it is. (I have a project that depends on
a sane auto_vec working).
So how does your usage look like? I can't really figure who'd need
deep copying of a container - note there's vec<>::copy at your
discretion.
A couple of alternatives to solving this are to use std::vector or
write an equivalent vector class just for GCC.
As said, can you show the usage that's impossible to do with
the current vec<>/auto_vec<>?
The test case in PR 90904 shows a trivial example. More generally,
using an auto_vec that depends on it being assignable (e.g., storing
an auto_vec in another container like hash_map or auto_vec itself)
is impossible. Using a plain vec requires manual memory management
and so is error-prone.
But more important, as a C++ code base, GCC should follow the best
practices for the language. Among the essential ones are using RAII
to manage resources and the Rule of Three (or Five in C++ 11): a class
that defines a dtor should also define a copy ctor and copy assignment
(and move ctor and move assignment in C++).
Martin
