On Wed, 2017-12-06 at 14:40 +0000, Richard Sandiford wrote:
> David Malcolm <dmalc...@redhat.com> writes:
> > On Wed, 2017-11-29 at 11:57 +0000, Richard Sandiford wrote:
> >
> > [...]
> >
> > I can't really comment on the representation ideas, but I'm always
> > happy to see new selftests...
> >
> > *************** test_labels ()
> > > *** 13954,13959 ****
> > > --- 14179,14350 ----
> > > ASSERT_FALSE (FORCED_LABEL (label_decl));
> > > }
> > >
> > > + /* Check that VECTOR_CST Y contains the elements in X. */
> > > +
> > > + static void
> > > + check_vector_cst (vec<tree> x, tree y)
> > > + {
> > > + for (unsigned int i = 0; i < x.length (); ++i)
> > > + ASSERT_EQ (wi::to_wide (x[i]), wi::to_wide (vector_cst_elt
> > > (y,
> > > i)));
> > > + }
> >
> > ...a couple of nits/suggestions:
> >
> > (a) How about renaming "x" to "expected"? Maybe rename "y" to
> > "actual"? (to better document the sense of the test).
>
> Good idea. I keep getting the ASSERT_* argument order for actual
> vs. expected confused, so having more explicit names would help.
FWIW, so do I. The ordering comes from googletest's API, which is what
the earliest version of the selftest code used (before Bernd persuaded
me to stop over-engineering it :) ), and changing it seemed like too
much trouble for not much gain.
That said I note that googletest's API now uses just "val1" and "val2"
for binary assertion macros, and their docs now say:
"Historical note: Before February 2016 *_EQ had a convention of calling
it as ASSERT_EQ(expected, actual), so lots of existing code uses this
order. Now *_EQ treats both parameters in the same way."
This seems to have been:
https://github.com/google/googletest/commit/f364e188372e489230ef4e44e1aec6bcb08f3acf
https://github.com/google/googletest/pull/713
(which FWIW talks about "expected, actual" as "Yoda ordering")
I guess we could make a similar change, but it seems low priority.
Better to at least support "actual" and "expected" as names in domain-
specific assertions, like your patch does for check_vector_cst.
> Done in the updated patch.
>
> However, I needed the patch below to use those names, since "actual"
> and
> "expected" are also used internally by the macros. I tried to
> protect
> other "user-level" names too.
>
> Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64le-linux-
> gnu.
> Bordering on obvious, but just in case: OK to install?
Looks good to me, though I'm not an official reviewer for the selftest
stuff.
[...]
Thanks
Dave
