Not very good strcpy implementation in cpython/Python/strdup.c

[wasiher]

BPO	41340
Nosy	@benjaminp, @methane, @shihai1991, @wasiher
PRs	bpo-41340: Removed strcpy from strdup function #21544 bpo-41340: Removed fallback implementation for strdup #21634

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2020-07-27.03:29:02.695>
created_at = <Date 2020-07-19.10:36:04.764>
labels = ['expert-C-API', '3.10', 'performance']
title = 'Not very good strcpy implementation in cpython/Python/strdup.c'
updated_at = <Date 2020-07-27.03:29:02.694>
user = 'https://github.com/wasiher'

bugs.python.org fields:

activity = <Date 2020-07-27.03:29:02.694>
actor = 'methane'
assignee = 'none'
closed = True
closed_date = <Date 2020-07-27.03:29:02.695>
closer = 'methane'
components = ['C API']
creation = <Date 2020-07-19.10:36:04.764>
creator = 'fj92f3jj923f923'
dependencies = []
files = []
hgrepos = []
issue_num = 41340
keywords = ['patch']
message_count = 11.0
messages = ['373955', '373976', '373986', '373989', '374199', '374241', '374302', '374319', '374345', '374353', '374359']
nosy_count = 4.0
nosy_names = ['benjamin.peterson', 'methane', 'shihai1991', 'fj92f3jj923f923']
pr_nums = ['21544', '21634']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue41340'
versions = ['Python 3.10']

[wasiher]

Hi, all!

strdup implementation inside cpython/Python/strdup.c is not the best one.
It calls strlen + strcpy, which is the same as calling strlen twice + memcpy.
So I replaced it by the call of strlen + memcpy.

It is easy to look any implementation in any library.
Here for example:
https://code.woboq.org/userspace/glibc/string/strdup.c.html

So I fixed it here:
#21544

[benjaminp]

I don't think we need to spend much effort on this implementation, since it will only be used if the system libc doesn't have a strdup already.

[wasiher]

Got it.
But the fix is quite easy as I see, and does not require much affort too.

[methane]

Can we just remove strdup.c? How about hypot.c?

[gvanrossum]

Either remove it or close this issue as won’t fix.

[shihai1991]

What's change for the performance benchmark?

[methane]

@fj92f3jj923f923 Would you update the PR to remove the strdup.c?

[gvanrossum]

There’s probably also something in configure.in that can switch this on.

[wasiher]

I can give a try with removing
Please, wait
Thanks everyone for giving a chance to make a PR :)

[wasiher]

Created a new PR for removal of strdup (Hope it is correct)
#21634
Closed old one

[methane]

New changeset 5798f78 by wasiher in branch 'master':
bpo-41340: Removed fallback implementation for strdup (GH-21634)
5798f78