On 7/13/21 4:02 PM, Martin Sebor wrote:
On 7/13/21 12:37 PM, Jason Merrill wrote:
On 7/13/21 10:08 AM, Jonathan Wakely wrote:
On Mon, 12 Jul 2021 at 12:02, Richard Biener wrote:
Somebody with more C++ knowledge than me needs to approve the
vec.h changes - I don't feel competent to assess all effects of the
change.
They look OK to me except for:
-extern vnull vNULL;
+static constexpr vnull vNULL{ };
Making vNULL have static linkage can make it an ODR violation to use
vNULL in templates and inline functions, because different
instantiations will refer to a different "vNULL" in each translation
unit.
The ODR says this is OK because it's a literal constant with the same
value (6.2/12.2.1).
But it would be better without the explicit 'static'; then in C++17
it's implicitly inline instead of static.
I'll remove the static.
But then, do we really want to keep vNULL at all? It's a weird
blurring of the object/pointer boundary that is also dependent on vec
being a thin wrapper around a pointer. In almost all cases it can be
replaced with {}; one exception is == comparison, where it seems to be
testing that the embedded pointer is null, which is a weird thing to
want to test.
The one use case I know of for vNULL where I can't think of
an equally good substitute is in passing a vec as an argument by
value. The only way to do that that I can think of is to name
the full vec type (i.e., the specialization) which is more typing
and less generic than vNULL. I don't use vNULL myself so I wouldn't
miss this trick if it were to be removed but others might feel
differently.
In C++11, it can be replaced by {} in that context as well.
If not, I'm all for getting rid of vNULL but with over 350 uses
of it left, unless there's some clever trick to make the removal
(mostly) effortless and seamless, I'd much rather do it independently
of this initial change. I also don't know if I can commit to making
all this cleanup.
I already have a patch to replace all but one use of vNULL, but I'll
hold off with it until after your patch.
Somewhat relatedly, use of vec<T> variables or fields seems almost
always a mistake, as they need explicit .release() that could be
automatic with auto_vec, and is easy to forget. For instance, the
recursive call in get_all_loop_exits returns a vec that is never
released. And I see a couple of leaks in the C++ front end as well.
I agree. The challenge I ran into with changing vec fields is with
code that uses the vec member as a reference to auto_vec. This is
the case in gcc/ipa-prop.h, for instance. Those instances could
be changed to auto_vec references or pointers but again it's a more
intrusive change than the simple replacements I was planning to make
in this first iteration.
So in summary, I agree with the changes you suggest. Given their
scope I'd prefer not to make them in the same patch, and rather make
them at some point in the future when I or someone else has the time
and energy. I'm running out.
Oh, absolutely.
Jason
