string/string.h: strsep(): Remove 'restrict'

Message ID 20240704221156.36228-2-alx@kernel.org
State New
Headers
Series string/string.h: strsep(): Remove 'restrict' |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Test passed

Commit Message

Alejandro Colomar July 4, 2024, 10:12 p.m. UTC
  This function doesn't need 'restrict' at all.  It seems it was added in
a (churny) patch that adds the qualifier to several functions, so it was
probably just an accident.

Reported-by: chux <https://codereview.stackexchange.com/users/29485/chux-reinstate-monica>
Link: <https://codereview.stackexchange.com/questions/173723/string-separator-similar-to-strtok/292820?noredirect=1#comment583130_292820>
Fixes: a6ff34d7b0b5 ("Update.")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 string/string.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
  

Comments

Paul Eggert July 4, 2024, 11:19 p.m. UTC | #1
On 7/5/24 00:12, Alejandro Colomar wrote:
> This function doesn't need 'restrict' at all.

Why not? Without 'restrict', strsep's two arguments could point at the 
same piece of storage, which could prevent some optimizations.
  
Alejandro Colomar July 4, 2024, 11:50 p.m. UTC | #2
Hi Paul,

On Fri, Jul 05, 2024 at 01:19:45AM GMT, Paul Eggert wrote:
> On 7/5/24 00:12, Alejandro Colomar wrote:
> > This function doesn't need 'restrict' at all.
> 
> Why not? Without 'restrict', strsep's two arguments could point at the same
> piece of storage, which could prevent some optimizations.

I don't see any optimizations that would benefit from that.

strsep(3) does (at most) two writes:

-  Zero the first delimiter found.
-  Update the input pointer.

And it's not possible to perform any of those write operations until
we've found the location, but when we've found the location, we don't
need 'delim' anymore.

A trivial implementation of strsep(3) would be:

	char *
	stpsep(char *s, const char *delim)
	{
		s += strcspn(s, delim);
		if (s == '\0')
			return NULL;

		strcpy(s++, "");
		return s;
	}

	char *
	strsep(char **s, const char *delim)
	{
		char  *p;

		p = *s;
		if (p == NULL)
			return NULL;
		*s = stpsep(p, delim);
		return p;
	}

Nothing is written in either string until strcspn(3) has finished, and
by then, delim is already dead.  I think it's impossible to come up with
an implementation that can take advantage of 'restrict', that is, an
implementation that would perform a write operation and still use delim
after that; don't you?

Have a lovely night!
Alex
  
Alejandro Colomar July 5, 2024, 11:50 a.m. UTC | #3
On Fri, Jul 05, 2024 at 12:12:41AM GMT, Alejandro Colomar wrote:
> This function doesn't need 'restrict' at all.  It seems it was added in
> a (churny) patch that adds the qualifier to several functions, so it was
> probably just an accident.
> 
> Reported-by: chux <https://codereview.stackexchange.com/users/29485/chux-reinstate-monica>
> Link: <https://codereview.stackexchange.com/questions/173723/string-separator-similar-to-strtok/292820?noredirect=1#comment583130_292820>
> Fixes: a6ff34d7b0b5 ("Update.")
> Signed-off-by: Alejandro Colomar <alx@kernel.org>
> ---
>  string/string.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/string/string.h b/string/string.h
> index d2d5c5f1f9..ee1f5d3aec 100644
> --- a/string/string.h
> +++ b/string/string.h
> @@ -468,8 +468,7 @@ extern void explicit_bzero (void *__s, size_t __n) __THROW __nonnull ((1))
>  
>  /* Return the next DELIM-delimited token from *STRINGP,
>     terminating it with a '\0', and update *STRINGP to point past it.  */
> -extern char *strsep (char **__restrict __stringp,

Hmmm, I should keep restrict in this parameter, since 'stringp' must be
a restricted pointer; it's '*stringp' which should be allowed to alias
'delim'.

See also: <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112833>

> -		     const char *__restrict __delim)
> +extern char *strsep (char **__stringp, const char *__delim)
>       __THROW __nonnull ((1, 2));
>  #endif
>  
> -- 
> 2.45.2
>
  

Patch

diff --git a/string/string.h b/string/string.h
index d2d5c5f1f9..ee1f5d3aec 100644
--- a/string/string.h
+++ b/string/string.h
@@ -468,8 +468,7 @@  extern void explicit_bzero (void *__s, size_t __n) __THROW __nonnull ((1))
 
 /* Return the next DELIM-delimited token from *STRINGP,
    terminating it with a '\0', and update *STRINGP to point past it.  */
-extern char *strsep (char **__restrict __stringp,
-		     const char *__restrict __delim)
+extern char *strsep (char **__stringp, const char *__delim)
      __THROW __nonnull ((1, 2));
 #endif