Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#4245 enhancement closed fixed (fixed)

Drop usage of cStringIO in _c_urlarg.c

Reported by: loewis Owned by:
Priority: normal Milestone: Python-3.x
Component: core Keywords: py3k
Cc: Branch: branches/curlarg-drop-cstringio-4245
(diff, github, buildbot, log)
Author: spiv Launchpad Bug:

Description

This patch drops usage of cStringIO from _c_urlarg.c, and replaces it with a regular string object. This is necessary to port the module to Python 3, as there won't be a cStringIO module anymore.

In addition, I also think this is a structural improvement, as using cStringIO is really overkill here. Please correct me if I'm wrong: unquote can only ever shrink the string. Therefore, it is possible to start with a buffer with a size equal to the input string, and can be guaranteed that the output will fit.

The patch uses _PyString_Resize. If that is no acceptable, I can rewrite it to use a separate buffer. Using _PyString_Resize has the advantage that it may avoid an extra copy at the end, namely if only few escaped characters had been in the URL (so that resizing the string doesn't need to reallocate the string object).

Attachments (1)

urlarg_nostringio.diff (2.1 KB) - added by loewis 5 years ago.

Download all attachments as: .zip

Change History (10)

Changed 5 years ago by loewis

comment:1 Changed 5 years ago by loewis

  • Keywords py3k added

comment:2 Changed 5 years ago by spiv

  • Keywords review added
  • Owner changed from glyph to spiv

comment:3 Changed 5 years ago by spiv

  • Keywords review removed

Thanks for the patch!

I think using _PyString_Resize is fine, it's clearly documented as a public API in the Python C API docs, even though the name's underscore prefix would suggest otherwise. This patch is probably even a small performance improvement. In general I'm pretty hesitant about touching this C code, even though it's a simple module it has had a surprising number of bugs over the years, but I can't see any flaw in this change.

The only thing missing from this change is a news entry (see ReviewProcess#Newsfiles). Apart from that I think this is fine to merge, so I'll write that news entry and merge it for you, as AFAIK you don't have committer privileges.

Btw, you might like to review our ReviewProcess page; you could have set the "review" keyword on all your py3k tickets because you provided patches, and you would have seen the requirement for news entries, etc.

comment:4 Changed 5 years ago by spiv

  • Author set to spiv
  • Branch set to branches/curlarg-drop-cstringio-4245

(In [28099]) Branching to 'curlarg-drop-cstringio-4245'

comment:5 Changed 5 years ago by spiv

  • Resolution set to fixed
  • Status changed from new to closed

(In [28105]) Merge curlarg-drop-cstringio-4245: Drop usage of cStringIO in _c_urlarg.c

Author: loewis
Reviewer: spiv
Fixes: #4245
Refs: #4246

Drop usage of cStringIO in _c_urlarg in favour of building (and resizing) a
PyString directly.

Thanks to martin@… for the patch.

comment:6 Changed 5 years ago by loewis

Thanks for pointing out the review process; I was unaware of it. This is indeed my first contribution to Twisted, and indeed, I'm not a committer.

comment:7 Changed 5 years ago by spiv

You're very welcome. Thanks again for contributing!

comment:8 Changed 4 years ago by glyph

  • Milestone set to Python-3.x

comment:9 Changed 4 years ago by <automation>

  • Owner spiv deleted
Note: See TracTickets for help on using tickets.