bpo-30717: add unicode grapheme cluster break algorithm #2673

Vermeille · 2017-07-11T23:12:53Z

I have added GraphemeBreakProperty to UnicodeData.
An automaton to compute the rules for breaking grapheme clusters according to TR29 is included. It passes all the tests provided in GraphemeBreakTests.txt.

https://bugs.python.org/issue30717

the-knights-who-say-ni · 2017-07-11T23:12:56Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

Vermeille · 2017-08-02T15:43:47Z

Hello? Someone here?

serhiy-storchaka · 2017-08-03T08:13:20Z

Modules/unicodedata.c

+            ++p->pos;
+        }
+    } else {
+        /* Raising of standard StopIteration exception with empty value. */


You can just return NULL without raising a StopIteration.

serhiy-storchaka · 2017-08-03T08:13:20Z

Modules/unicodedata.c

+    GraphemeClusterIterator *p = (GraphemeClusterIterator *)self;
+    int kind = PyUnicode_KIND(p->str);
+    void *pstr = PyUnicode_DATA(p->str);
+    if (PyUnicode_READ(kind, pstr, p->pos)) {


The iteration stops on the first NUL character. What about strings containing embedded NUL characters?

Would it be a legal (text) string then? I assumed no.

Why not? NUL characters don't have special meaning in Python strings.

Silent truncating the string on the first NUL character would look like a bug.

Alright, it was just my C++ biais :) fixing.

serhiy-storchaka · 2017-08-03T08:13:20Z

Modules/grapheme_cluster_break_automaton.h

+    STATE_Any,
+} GCBState;
+
+static GCBState GRAPH_CLUSTER_AUTOMATON[15][20] = {


This may be a const array.

There are only 19 values in GraphemeClusterBreakType, but the size of the second dimension is 20.

serhiy-storchaka · 2017-08-03T08:13:20Z

Tools/unicode/makeunicodedata.py

    if not os.path.exists(local):
        import urllib.request
        if version == '3.2.0':
            # irregular url structure
            url = 'http://www.unicode.org/Public/3.2-Update/' + local
        else:
            url = ('http://www.unicode.org/Public/%s/ucd/'+template) % (version, '')
+        print(url)


Debugging remnants?

methane · 2017-08-03T10:08:12Z

Modules/unicodedata.c

+};
+
+/*[clinic input]
+unicodedata.UCD.break_graphemes


Personally, I prefer iter_graphemes because it's clear to return an iterator.

I like it better too! Fixing!

methane · 2017-08-03T10:08:12Z

Modules/unicodedata.c

+    sizeof(GraphemeClusterIterator),       /*tp_basicsize*/
+    0,                         /*tp_itemsize*/
+    (destructor)GCI_Del,       /*tp_dealloc*/
+    0,                         /*tp_print*/


Please use designated initializers for new types.

... .tp_dealloc = (destructor)GCI_Del, .tp_flags = Py_TPFLAGS_DEFAULT, .tp_doc = "Internal grapheme cluster iterator object", .tp_iter = GCI_iter, .tp_iternext = GCI_iternext };

methane · 2017-08-03T10:08:12Z

Modules/unicodedata.c

+typedef struct {
+    PyObject_HEAD
+    PyObject* str;
+    int pos;


Use py_ssize_t instead of int.

serhiy-storchaka · 2017-08-03T10:56:45Z

Modules/unicodedata.c

+    GraphemeClusterIterator *p = (GraphemeClusterIterator *)self;
+    int kind = PyUnicode_KIND(p->str);
+    void *pstr = PyUnicode_DATA(p->str);
+    if (PyUnicode_READ(kind, pstr, p->pos)) {


Why not? NUL characters don't have special meaning in Python strings.

Silent truncating the string on the first NUL character would look like a bug.

serhiy-storchaka · 2017-08-03T10:56:45Z

Modules/unicodedata.c

+typedef struct {
+    PyObject_HEAD
+    PyObject* str;
+    int pos;


Py_ssize_t

fixed in the upcoming patch

serhiy-storchaka · 2017-08-03T10:56:45Z

Modules/unicodedata.c

+    0,  /* tp_clear */
+    0,  /* tp_richcompare */
+    0,  /* tp_weaklistoffset */
+    GCI_iter,  /* tp_iter: __iter__() method */


PyObject_SelfIter

fixed in the upcoming patch

serhiy-storchaka · 2017-08-03T10:56:45Z

Modules/unicodedata.c

+    int pos;
+} GraphemeClusterIterator;
+
+void GCI_Del(PyObject* x)


The return type should be on separate line (see PEP 7).

The function should be static.

Usually destructors have the dealloc suffix.

You can use the pointer to GraphemeClusterIterator.

And it is more common to add a space before * than after it.

static void GCI_dealloc(GraphemeClusterIterator *it)

fixed in the upcoming patch

serhiy-storchaka · 2017-08-03T10:56:45Z

Modules/unicodedata.c

+
+#include "grapheme_cluster_break_automaton.h"
+
+PyObject* GCI_iternext(PyObject *self)


Most comments about the destructor are applicable to this method.

fixed in the upcoming patch

serhiy-storchaka · 2017-08-03T10:56:45Z

Modules/unicodedata.c

+    0,                         /*tp_getattro*/
+    0,                         /*tp_setattro*/
+    0,                         /*tp_as_buffer*/
+    Py_TPFLAGS_DEFAULT,


Since the str field can be not exact Unicode object, but an instance of the subclass of the str class, the iterator should be a GC type. Add Py_TPFLAGS_HAVE_GC, implement tp_traverse, use PyObject_GC_New() and PyObject_GC_Del(), add PyObject_GC_Untrack() in the destructor. See https://docs.python.org/3/c-api/gcsupport.html.

So, should I remove the current tp_dealloc and replace it with a tp_clear? Should the tp_dealloc be kept? Reading the doc, it seems that tp_traverse and tp_clear are related. Can you make that a little bit clearer :) ? Thanks!

I've done something on my own that seems to work, but you should definitely double check that.

serhiy-storchaka · 2017-08-03T10:56:45Z

Modules/unicodedata.c

+    0,                         /*tp_setattro*/
+    0,                         /*tp_as_buffer*/
+    Py_TPFLAGS_DEFAULT,
+    "Internal grapheme cluster iterator object.",           /* tp_doc */


I think the words "internal" and "object" are redundant.

"Internal", "iterator" and "object" are all redundant. "Grapheme cluster iterator" seems just right. What do you think?

serhiy-storchaka · 2017-08-03T10:56:45Z

Modules/unicodedata.c

+};
+
+/*[clinic input]
+unicodedata.UCD.break_graphemes


I think it would be useful to support iterating a substring. Add optional start and end parameters as in str.index() or re.Pattern.finditer().

fixed in the upcoming patch

Vermeille · 2018-01-11T10:47:54Z

Sorry for the long wait.

Are we good concerning the changes? Anything to add?

brettcannon · 2018-02-02T22:04:50Z

To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request.

If/when the requested changes have been made, please leave a comment that says, I have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review.

csabella · 2020-05-23T16:46:41Z

@Vermeille, please take a look at the most recent comments on the bug tracker for this issue. It looks like the suggested path forward is different than the solution you proposed here. Thanks!

github-actions · 2022-02-20T00:09:47Z

This PR is stale because it has been open for 30 days with no activity.

github-actions · 2022-08-29T00:13:26Z

This PR is stale because it has been open for 30 days with no activity.

add unicodedata.grapheme_cluster_break()

b79f969

the-knights-who-say-ni added the CLA not signed label Jul 11, 2017

Vermeille changed the title ~~WIP: add grapheme cluster break algorithm~~ bpo-30717: WIP: add grapheme cluster break algorithm Jul 11, 2017

generate unicodedata.c.h with clinic

c9a4211

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Jul 13, 2017

patchcheck

7f56b78

Vermeille force-pushed the grapheme_cluster_break branch from 0f82f82 to 62fd6e0 Compare Jul 13, 2017

Vermeille changed the title ~~bpo-30717: WIP: add grapheme cluster break algorithm~~ bpo-30717: add grapheme cluster break algorithm Jul 14, 2017

Vermeille changed the title ~~bpo-30717: add grapheme cluster break algorithm~~ bpo-30717: add unicode grapheme cluster break algorithm Jul 14, 2017

add the grapheme cluster break automaton

a47de54

Vermeille force-pushed the grapheme_cluster_break branch from 62fd6e0 to a47de54 Compare Jul 14, 2017

add my name to Misc/ACKS

c152171

serhiy-storchaka reviewed Aug 3, 2017

View changes

serhiy-storchaka added the type-feature A feature request or enhancement label Aug 3, 2017

code review fixes

b103be7

methane requested changes Aug 3, 2017

View changes

rename break_graphemes to iter_graphemes

c9848e2

serhiy-storchaka reviewed Aug 3, 2017

View changes

Vermeille added 2 commits Aug 3, 2017

make GraphemeClusterIterator a GC type

2dee91e

allow iterating only over a range of indices

a5b3c10

brettcannon added the awaiting changes label Feb 2, 2018

github-actions bot added the stale Stale PR or inactive for long period of time. label Feb 20, 2022

ezio-melotti removed the CLA signed label Jul 13, 2022

github-actions bot removed the stale Stale PR or inactive for long period of time. label Jul 28, 2022

github-actions bot added the stale Stale PR or inactive for long period of time. label Aug 29, 2022

bpo-30717: add unicode grapheme cluster break algorithm #2673

bpo-30717: add unicode grapheme cluster break algorithm #2673

Vermeille commented Jul 11, 2017 •

edited by bedevere-bot

the-knights-who-say-ni commented Jul 11, 2017

Vermeille commented Aug 2, 2017

serhiy-storchaka Aug 3, 2017

serhiy-storchaka Aug 3, 2017

Vermeille Aug 3, 2017

serhiy-storchaka Aug 3, 2017

Vermeille Aug 3, 2017

serhiy-storchaka Aug 3, 2017

Vermeille Jan 10, 2018

serhiy-storchaka Aug 3, 2017

methane Aug 3, 2017

Vermeille Aug 3, 2017

methane Aug 3, 2017

Vermeille Jan 10, 2018

methane Aug 3, 2017

Vermeille Jan 10, 2018

serhiy-storchaka Aug 3, 2017

serhiy-storchaka Aug 3, 2017

Vermeille Aug 3, 2017

serhiy-storchaka Aug 3, 2017

Vermeille Aug 3, 2017

serhiy-storchaka Aug 3, 2017

Vermeille Aug 3, 2017

serhiy-storchaka Aug 3, 2017

Vermeille Aug 3, 2017

serhiy-storchaka Aug 3, 2017

Vermeille Aug 3, 2017

Vermeille Aug 3, 2017

serhiy-storchaka Aug 3, 2017

Vermeille Aug 3, 2017

serhiy-storchaka Aug 3, 2017

Vermeille Aug 3, 2017

Vermeille commented Jan 11, 2018

brettcannon commented Feb 2, 2018

csabella commented May 23, 2020

github-actions bot commented Feb 20, 2022

github-actions bot commented Aug 29, 2022


		#include "grapheme_cluster_break_automaton.h"

		PyObject* GCI_iternext(PyObject *self)

bpo-30717: add unicode grapheme cluster break algorithm #2673

Are you sure you want to change the base?

bpo-30717: add unicode grapheme cluster break algorithm #2673

Conversation

Vermeille commented Jul 11, 2017 • edited by bedevere-bot

the-knights-who-say-ni commented Jul 11, 2017

Vermeille commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vermeille commented Jan 11, 2018

brettcannon commented Feb 2, 2018

csabella commented May 23, 2020

github-actions bot commented Feb 20, 2022

github-actions bot commented Aug 29, 2022

Vermeille commented Jul 11, 2017 •

edited by bedevere-bot