Jakub Jelinek <ja...@redhat.com> writes:
> On Mon, Jan 20, 2020 at 08:59:20AM +0000, Iain Sandoe wrote:
>> Hi Bin,
>>
>> bin.cheng <bin.ch...@linux.alibaba.com> wrote:
>>
>> > gcc/cp
>> > 2020-01-20 Bin Cheng <bin.li...@linux.alibaba.com>
>> >
>> > * coroutines.cc (build_co_await): Skip getting complete type for
>> > void.
>> >
>> > gcc/testsuite
>> > 2020-01-20 Bin Cheng <bin.li...@linux.alibaba.com>
>> >
>> > * g++.dg/coroutines/co-await-void_type.C: New
>> > test.<0001-Fix-ICE-when-co_awaiting-on-void-type.patch>
>>
>> This LGTM, (borderline obvious, in fact) but you will need to wait for a C++
>> maintainer to approve,
>
> The patch actually looks wrong to me, there is nothing magic about void
> type here, it will ICE on any other incomplete type, because
> complete_type_or_else returns NULL if the type is incomplete.
>
> So, I think you want instead:
> tree o_type = complete_type_or_else (TREE_TYPE (o), o);
> + if (o_type == NULL_TREE)
> + return error_mark_node;
> if (TREE_CODE (o_type) != RECORD_TYPE)
>
> or similar (the diagnostics should be emitted already, so I don't see
> further point to emit it once again).
>
> But it also means that other uses of complete_type_or_else look broken:
>
> /* Complete this, we're going to use it. */
> coro_info->handle_type = complete_type_or_else (handle_type, fndecl);
> /* Diagnostic would be emitted by complete_type_or_else. */
> if (coro_info->handle_type == error_mark_node)
> return false;
>
> or
>
> if (!COMPLETE_TYPE_P (actual_type))
> actual_type = complete_type_or_else (actual_type, *stmt);
>
> if (TREE_CODE (actual_type) == REFERENCE_TYPE)
> actual_type = build_pointer_type (TREE_TYPE (actual_type));
>
> where I think the first one should check for handle_type being NULL_TREE
> rather than error_mark_node, and the latter should handle the case of
> it returning NULL_TREE.
>
> Right below the last hunk, I've noticed:
>
> size_t namsize = sizeof ("__parm.") + IDENTIFIER_LENGTH (pname) + 1;
> char *buf = (char *) alloca (namsize);
> snprintf (buf, namsize, "__parm.%s", IDENTIFIER_POINTER (pname));
>
> Do you really need one byte extra? I mean, sizeof ("__parm.") already
> includes +1 for the terminating '\0', so unless you append something to
> the buffer later, I don't see the point for the + 1.
> And the middle line could be written as
> char *buf = XALLOCAVEC (char, namsize);
FWIW, there's also the option of:
char *buf = ACONCAT ((""__parm.", IDENTIFIER_POINTER (pname), NULL));
But using alloca/ACONCAT is probably a bad idea for unconstrained
user-supplied names, if that's what we have here.
Richard
