Merge lp:~danilo/pygettextpo/simplify into lp:pygettextpo

Proposed by Данило Шеган
Status: Needs review
Proposed branch: lp:~danilo/pygettextpo/simplify
Merge into: lp:pygettextpo
Prerequisite: lp:~danilo/pygettextpo/pep8-fixes
Diff against target: 169 lines (+40/-92)
1 file modified
gettextpo.c (+40/-92)
To merge this branch: bzr merge lp:~danilo/pygettextpo/simplify
Reviewer Review Type Date Requested Status
Benji York (community) code Approve
Review via email: mp+92619@code.launchpad.net

Description of the change

Simplify the C code behind simple set_msg* methods to facilitate code reuse.

Since I'd like to extend it to support all the other current features of libgettextpo, and a bunch of other methods would need the same code for executing a bunch of actions (set_msgctxt, set_extracted_comments...).

Alternative approach for this would have been to define a macro, which might be faster (no passing around of function pointers), but it would have resulted in larger binaries and would have been uglier (\ continuation lines and such).

'make check' confirms no tests are broken (all the modified methods are tested in a basic way, but that should be sufficient for the type of changes here).

To post a comment you must log in.
Revision history for this message
Benji York (benji) wrote :

This branch looks good. I just have a couple of cosmetic things to
mention.

Is the space between "*" and "setter" intentional in the below (lines
11-12 of the diff):

    message_set_field(PyPoMessage *self, PyObject *args, const char *field,
                            po_value_setter_t * setter)

Also, is it just me or is the indentation of the second line above (line
12 of the diff) odd? I don't know the prevailing style in the
surrounding code, but I would expect an indentation of 4 spaces, 8
spaces, or aligning the start of the second line with the open
parenthesis in line 11.

Again, I'm not familiar with the style guides of pygettextpo, but most C
code bases try to avoid non-braced if blocks like those on line 18, 21,
and 26.

Oh, I see now that the code was mostly just moved, so perhaps the
one-line-bodies are OK.

review: Approve (code)
Revision history for this message
Colin Watson (cjwatson) wrote :

I've converted pygettextpo to git and merged this branch (though I can't mark this MP as Merged):

  https://git.launchpad.net/pygettextpo/commit/?id=85b497cf0eaf6e7d39d557fac551d63cd009ee38

Unmerged revisions

28. By Данило Шеган

Merge pep8-fixes.

27. By Данило Шеган

Remove "helper" from the helper function name: it confused me a bare month after I introduced it because I read it as "helper message" instead of a "helper function on message class".

26. By Данило Шеган

Reuse the new helper function in other similar methods.

25. By Данило Шеган

Extract po message setter pattern into a helper method.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'gettextpo.c'
2--- gettextpo.c 2009-06-02 15:31:25 +0000
3+++ gettextpo.c 2012-02-11 12:09:17 +0000
4@@ -533,35 +533,46 @@
5 return res;
6 }
7
8+typedef void (po_value_setter_t)(po_message_t, const char *);
9+
10+static PyObject *
11+_message_set_field(PyPoMessage *self, PyObject *args, const char *field,
12+ po_value_setter_t * setter)
13+{
14+ const char *value;
15+ PyObject *object;
16+ PyObject *string;
17+
18+ if (!PyArg_ParseTuple(args, field, &object))
19+ return NULL;
20+
21+ if (object == Py_None) {
22+ (*setter)(self->msg, NULL);
23+ } else {
24+ string = get_pystring_from_pyobject(object);
25+
26+ if (string == NULL)
27+ /* Got an exception */
28+ return NULL;
29+ else {
30+ value = PyString_AsString(string);
31+ (*setter)(self->msg, value);
32+ Py_DECREF(string);
33+ }
34+ }
35+
36+ Py_RETURN_NONE;
37+}
38+
39+
40 PyDoc_STRVAR(doc_pypo_message_set_msgid,
41 "M.set_msgid(msgid) -> None. Set the msgid for this PoMessage");
42
43 static PyObject *
44 pypo_message_set_msgid(PyPoMessage *self, PyObject *args)
45 {
46- const char *msgid;
47- PyObject *object;
48- PyObject *string;
49-
50- if (!PyArg_ParseTuple(args, "O:set_msgid", &object))
51- return NULL;
52-
53- if (object == Py_None) {
54- po_message_set_msgid(self->msg, NULL);
55- } else {
56- string = get_pystring_from_pyobject(object);
57-
58- if (string == NULL)
59- /* Got an exception */
60- return NULL;
61- else {
62- msgid = PyString_AsString(string);
63- po_message_set_msgid(self->msg, msgid);
64- Py_DECREF(string);
65- }
66- }
67-
68- Py_RETURN_NONE;
69+ return _message_set_field(self, args, "O:set_msgid",
70+ &po_message_set_msgid);
71 }
72
73 PyDoc_STRVAR(doc_pypo_message_set_msgid_plural,
74@@ -571,29 +582,8 @@
75 static PyObject *
76 pypo_message_set_msgid_plural(PyPoMessage *self, PyObject *args)
77 {
78- const char *msgid;
79- PyObject *object;
80- PyObject *string;
81-
82- if (!PyArg_ParseTuple(args, "O:set_msgid_plural", &object))
83- return NULL;
84-
85- if (object == Py_None) {
86- po_message_set_msgid_plural(self->msg, NULL);
87- } else {
88- string = get_pystring_from_pyobject(object);
89-
90- if (string == NULL)
91- /* Got an exception */
92- return NULL;
93- else {
94- msgid = PyString_AsString(string);
95- po_message_set_msgid_plural(self->msg, msgid);
96- Py_DECREF(string);
97- }
98- }
99-
100- Py_RETURN_NONE;
101+ return _message_set_field(self, args, "O:set_msgid_plural",
102+ &po_message_set_msgid_plural);
103 }
104
105 PyDoc_STRVAR(doc_pypo_message_set_msgstr,
106@@ -602,29 +592,8 @@
107 static PyObject *
108 pypo_message_set_msgstr(PyPoMessage *self, PyObject *args)
109 {
110- const char *msgstr;
111- PyObject *object;
112- PyObject *string;
113-
114- if (!PyArg_ParseTuple(args, "O:set_msgstr", &object))
115- return NULL;
116-
117- if (object == Py_None) {
118- po_message_set_msgstr(self->msg, NULL);
119- } else {
120- string = get_pystring_from_pyobject(object);
121-
122- if (string == NULL)
123- /* Got an exception */
124- return NULL;
125- else {
126- msgstr = PyString_AsString(string);
127- po_message_set_msgstr(self->msg, msgstr);
128- Py_DECREF(string);
129- }
130- }
131-
132- Py_RETURN_NONE;
133+ return _message_set_field(self, args, "O:set_msgstr",
134+ &po_message_set_msgstr);
135 }
136
137 PyDoc_STRVAR(doc_pypo_message_set_msgstr_plural,
138@@ -675,29 +644,8 @@
139 static PyObject *
140 pypo_message_set_comments(PyPoMessage *self, PyObject *args)
141 {
142- const char *comments;
143- PyObject *object;
144- PyObject *string;
145-
146- if (!PyArg_ParseTuple(args, "O:set_comments", &object))
147- return NULL;
148-
149- if (object == Py_None) {
150- po_message_set_comments(self->msg, NULL);
151- } else {
152- string = get_pystring_from_pyobject(object);
153-
154- if (string == NULL)
155- /* Got an exception */
156- return NULL;
157- else {
158- comments = PyString_AsString(string);
159- po_message_set_comments(self->msg, comments);
160- Py_DECREF(string);
161- }
162- }
163-
164- Py_RETURN_NONE;
165+ return _message_set_field(self, args, "O:set_comments",
166+ &po_message_set_comments);
167 }
168
169 PyDoc_STRVAR(doc_pypo_message_set_format,

Subscribers

People subscribed via source and target branches