On 7/30/21 9:06 AM, Jason Merrill wrote:
On 7/27/21 2:56 PM, Martin Sebor wrote:
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575690.html
Are there any other suggestions or comments or is the latest revision
okay to commit?
OK.
I had to make a few more adjustments to fix up code that's snuck
in since I last tested the patch. I committed r12-2776 after
retesting on x86_64-linux.
With the cleanup out of the way I'll resubmit the copy ctor patch
next.
Martin
On 7/20/21 12:34 PM, Martin Sebor wrote:
On 7/14/21 10:23 AM, Jason Merrill wrote:
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.
I have just pushed r12-2418:
https://gcc.gnu.org/pipermail/gcc-cvs/2021-July/350886.html
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.
Okay.
+ 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&?
Calling to_vec() here isn't necessary so I've removed it.
- 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?
Sure, that works too.
Attached is what's left of the original changes now that r12-2418
has been applied.
Martin
