On 11/26/18 7:03 AM, Martin Liška wrote:
> On 11/23/18 6:10 PM, Martin Sebor wrote:
>> On 11/23/18 8:06 AM, Martin Liška wrote:
>>> Hi.
>>>
>>> It's patch proposal that suggests to use an enum instead of 'int endp' for
>>> functions that expand memory move builtins. I've touch the code multiple
>>> times
>>> and it always take me time to realize what the magic numbers 0, 1, 2 mean.
>>>
>>> Does the suggestion make sense? Do you like enum values? Can it be
>>> installed now?
>>> If so I can finish the patch (few targets must be ported and ChangeLog
>>> entry is missing).
>> I think an enum will make the code much more readable.
>>
>> I'm not sure the name choices are the most intuitive. Maybe
>> something like this instead?
>>
>> RETURN_BEGIN, RETURN_END, RETURN_END_MINUS_1
>>
>> (I suggest BEGIN rather than START because of C++ iterators, but
>> I see the bigger readability gain in using RETURN over POINTER;
>> IMO, it would be also nice to rename endp to retp or retmode
>> or something like that).
>>
>> For the RETURN_END_MINUS_1 comment, I would just should say
>> "return a pointer to the terminating null byte of the string,"
>> rather than "to the end of it." (The latter might seem to
>> contradict RETURN_END).
>>
>> Martin
> Thank you Martin for feedback. I'm sending tested version of the patch
> that survives bootstrap on x86_64-linux-gnu.
>
> Ready for trunk?
> Martin
>
>
> 0001-Come-up-with-memop_ret-enum-instead-of-int-endp-for-.patch
>
> From 1709326b01079cfca299e2b57be18bd616954bf8 Mon Sep 17 00:00:00 2001
> From: marxin <mli...@suse.cz>
> Date: Fri, 23 Nov 2018 15:51:05 +0100
> Subject: [PATCH] Come up with memop_ret enum instead of int endp for memory
> operations.
>
> gcc/ChangeLog:
>
> 2018-11-26 Martin Liska <mli...@suse.cz>
>
> * asan.c (asan_emit_stack_protection): Use new enum values
> instead of int constants.
> * builtins.c (expand_builtin_memory_copy_args): Replace int
> type with memop_ret enum type.
> (expand_builtin_mempcpy_args): Likewise.
> (expand_builtin_memcpy): Use new enum values
> instead of int constants. Likewise.
> (expand_builtin_mempcpy): Likewise.
> (expand_movstr): Likewise.
> (expand_builtin_strcpy_args): Likewise.
> (expand_builtin_stpcpy_1): Likewise.
> (expand_builtin_strncpy): Likewise.
> (expand_builtin_memset_args): Likewise.
> * expr.c (move_by_pieces_d::finish_endp): Rename to ...
> (move_by_pieces_d::finish_retmode): ... this.
> (move_by_pieces): Change last argument type to memop_ret.
> (store_by_pieces): Use new enum values
> instead of int constants.
> (emit_block_move_hints): Likewise.
> (emit_push_insn): Likewise.
> (store_expr): Likewise.
> * expr.h (store_by_pieces): Change int to newly added enum
> type.
> * rtl.h (enum memop_ret): Define.
> (move_by_pieces): Use the enum type.
> ---
> gcc/asan.c | 2 +-
> gcc/builtins.c | 73 +++++++++++++++++++++++++-------------------------
> gcc/expr.c | 53 +++++++++++++++++-------------------
> gcc/expr.h | 2 +-
> gcc/rtl.h | 17 +++++++++++-
> 5 files changed, 79 insertions(+), 68 deletions(-)
>
> rtx
> store_by_pieces (rtx to, unsigned HOST_WIDE_INT len,
> rtx (*constfun) (void *, HOST_WIDE_INT, scalar_int_mode),
> - void *constfundata, unsigned int align, bool memsetp, int endp)
> + void *constfundata, unsigned int align, bool memsetp,
> + memop_ret retmode)
> {
> if (len == 0)
> {
> - gcc_assert (endp != 2);
> + gcc_assert (retmode != 2);
Don't you want this to be retmode != RETURN_END_MINUS_1
With that nit fixed, I think this is OK.
jeff
