Re: [PING][PATCH] define auto_vec copy ctor and assignment (PR 90904)

[Jason Merrill <jason@redhat.com>]

On 7/14/21 10:46 AM, Martin Sebor wrote:
> On 7/13/21 9:39 PM, Jason Merrill wrote:
>> 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.
> 
> Cool.  I thought I'd tried { } here but I guess not.
> 
>>
>>> 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.
> 
> So what's the next step?  The patch only removes a few uses of vNULL
> but doesn't add any.  Is it good to go as is (without the static and
> with the additional const changes Richard suggested)?  This patch is
> attached to my reply to Richard:
> https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575199.html

As Richard wrote:

> The pieces where you change vec<> passing to const vec<>& and the few
> where you change vec<> * to const vec<> * are OK - this should make the
> rest a smaller piece to review.

Please go ahead and apply those changes and send a new patch with the 
remainder of the changes.

A few other comments:

> -				       omp_declare_simd_clauses);
> +				       *omp_declare_simd_clauses);

Instead of doing this indirection in all of the callers, let's change 
c_finish_omp_declare_simd to take a pointer as well, and do the 
indirection in initializing a reference variable at the top of the function.

> +    sched_init_luids (bbs.to_vec ());
> +    haifa_init_h_i_d (bbs.to_vec ());

Why are these to_vec changes needed when you are also changing the 
functions to take const&?

> -  vec<tree> checks = LOOP_VINFO_CHECK_NONZERO (loop_vinfo);
> +  vec<tree> checks = LOOP_VINFO_CHECK_NONZERO (loop_vinfo).to_vec ();

Why not use a reference here and in other similar spots?

Jason