This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][RFC] Come up with memop_ret enum instead of int endp for memory operations.


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 <mliska@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  <mliska@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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]