On Mon, Oct 02, 2023 at 01:38:53PM +0100, Tamar Christina wrote:
> Hi All,
>
> I recently committed a patch that uses a nested std::pair in the second
> argument.
> It temporarily adds a second ranking variable for sorting and then later
> drops it.
>
> This hits the newly added assert in vec.h. This assert made some relaxation
> for
> std::pair but doesn't allow this case through. The patch allows a recursive
> std::pair in the second argument which fixes bootstrap.
I must say I still don't understand why using a
struct ifcvt_arg_entry { tree arg; unsigned len, occur; };
with comments describing what the members mean wouldn't be a better fix,
in the sorting function what exactly means x{1,2}.second.first and
x{1,2}.second.second isn't easily understandable, neither from the
identifiers nor from any comments.
Seems because you use 2 separate vectors, one with just tree elements and
another with those tree elements + 2 unsigned values cached from it for the
sorting purpose and then rewrite the original tree vector after sorting, I
don't really see why nested std::pair would be a better match for it than
a named structure. Furthermore, why populate args first, then compute
the extra 2 integers in another loop pushing to argsKV and finally overwrite
args with sorted values? Can't the first loop push tree with the 2 integers
already? And what is the point of not using this structure later on when
both args and argsKV vectors are live until the end of the same function?
Can't you either pass that argsKV to others, having just one vector, or
at least release the other vector when you don't really need it?
Formatting style, swap? arg1 : arg0 isn't correctly formatted, missing space
before ?.
Also, ArgEntry is CamelCase which we (usually) don't use in GCC and
additionally doesn't seem to be unique enough for ODR purposes.
Ditto argsKV.
> It should also still maintain the invariant that was being tested here since
> the nested arguments should still be trivially copyable.
>
> Bootstrapped on aarch64-none-linux-gnu, x86_64-linux-gnu, and no issues.
>
> Ok for master?
>
> Thanks,
> Tamar
>
> gcc/ChangeLog:
>
> vec.h (struct is_trivially_copyable_or_pair): Check recursively in
Missing "* " above.
> second arg.
Jakub
