From 1c46e72eeee526eb07f1e6bff9dd7e4a7dac4003 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Thu, 30 Dec 2021 08:54:42 -0800 Subject: [PATCH] Addressed PR comments. --- python/map.c | 96 +++++++++++++++++++++++------------------------ python/protobuf.c | 16 +++++--- upb/msg.c | 1 - 3 files changed, 59 insertions(+), 54 deletions(-) diff --git a/python/map.c b/python/map.c index b374f7f9cf..7e5c618c8a 100644 --- a/python/map.c +++ b/python/map.c @@ -103,7 +103,7 @@ void PyUpb_MapContainer_Reify(PyObject* _self, upb_map* map) { PyUpb_ObjCache_Add(map, &self->ob_base); Py_DECREF(self->ptr.parent); self->ptr.map = map; // Overwrites self->ptr.parent. - self->field = self->field & ~(uintptr_t)1; + self->field &= ~(uintptr_t)1; assert(!PyUpb_MapContainer_IsStub(self)); } @@ -153,6 +153,29 @@ int PyUpb_MapContainer_AssignSubscript(PyObject* _self, PyObject* key, return 0; } +PyObject* PyUpb_MapContainer_Subscript(PyObject* _self, PyObject* key) { + PyUpb_MapContainer* self = (PyUpb_MapContainer*)_self; + upb_map* map = PyUpb_MapContainer_GetIfWritable(self); + const upb_fielddef* f = PyUpb_MapContainer_GetField(self); + const upb_msgdef* entry_m = upb_fielddef_msgsubdef(f); + const upb_fielddef* key_f = upb_msgdef_field(entry_m, 0); + const upb_fielddef* val_f = upb_msgdef_field(entry_m, 1); + upb_arena* arena = PyUpb_Arena_Get(self->arena); + upb_msgval u_key, u_val; + if (!PyUpb_PyToUpb(key, key_f, &u_key, arena)) return NULL; + if (!map || !upb_map_get(map, u_key, &u_val)) { + map = PyUpb_MapContainer_AssureWritable(self); + upb_arena* arena = PyUpb_Arena_Get(self->arena); + if (upb_fielddef_issubmsg(val_f)) { + u_val.msg_val = upb_msg_new(upb_fielddef_msgsubdef(val_f), arena); + } else { + memset(&u_val, 0, sizeof(u_val)); + } + upb_map_set(map, u_key, u_val, arena); + } + return PyUpb_UpbToPy(u_val, val_f, self->arena); +} + PyObject* PyUpb_MapContainer_Contains(PyObject* _self, PyObject* key) { PyUpb_MapContainer* self = (PyUpb_MapContainer*)_self; upb_map* map = PyUpb_MapContainer_GetIfWritable(self); @@ -171,8 +194,8 @@ PyObject* PyUpb_MapContainer_Contains(PyObject* _self, PyObject* key) { PyObject* PyUpb_MapContainer_Clear(PyObject* _self, PyObject* key) { PyUpb_MapContainer* self = (PyUpb_MapContainer*)_self; - upb_map* map = PyUpb_MapContainer_GetIfWritable(self); - if (map) upb_map_clear(map); + upb_map* map = PyUpb_MapContainer_AssureWritable(self); + upb_map_clear(map); Py_RETURN_NONE; } @@ -197,14 +220,12 @@ static PyObject* PyUpb_MapContainer_Get(PyObject* _self, PyObject* args, if (!PyUpb_PyToUpb(key, key_f, &u_key, arena)) return NULL; if (map && upb_map_get(map, u_key, &u_val)) { return PyUpb_UpbToPy(u_val, val_f, self->arena); - } else { - if (default_value) { - Py_INCREF(default_value); - return default_value; - } else { - Py_RETURN_NONE; - } } + if (default_value) { + Py_INCREF(default_value); + return default_value; + } + Py_RETURN_NONE; } static PyObject* PyUpb_MapContainer_GetEntryClass(PyObject* _self, @@ -254,16 +275,21 @@ static PyObject* PyUpb_MapContainer_Repr(PyObject* _self) { upb_map* map = PyUpb_MapContainer_GetIfWritable(self); PyObject* dict = PyDict_New(); if (map) { - size_t iter = UPB_MAP_BEGIN; const upb_fielddef* f = PyUpb_MapContainer_GetField(self); const upb_msgdef* entry_m = upb_fielddef_msgsubdef(f); const upb_fielddef* key_f = upb_msgdef_field(entry_m, 0); const upb_fielddef* val_f = upb_msgdef_field(entry_m, 1); + size_t iter = UPB_MAP_BEGIN; while (upb_mapiter_next(map, &iter)) { PyObject* key = PyUpb_UpbToPy(upb_mapiter_key(map, iter), key_f, self->arena); PyObject* val = PyUpb_UpbToPy(upb_mapiter_value(map, iter), val_f, self->arena); + if (!key || !val) { + Py_XDECREF(key); + Py_XDECREF(val); + return NULL; + } PyDict_SetItem(dict, key, val); Py_DECREF(key); Py_DECREF(val); @@ -274,47 +300,21 @@ static PyObject* PyUpb_MapContainer_Repr(PyObject* _self) { return repr; } -PyObject* PyUpb_MapContainer_Subscript(PyObject* _self, PyObject* key) { - PyUpb_MapContainer* self = (PyUpb_MapContainer*)_self; - upb_map* map = PyUpb_MapContainer_GetIfWritable(self); - const upb_fielddef* f = PyUpb_MapContainer_GetField(self); - const upb_msgdef* entry_m = upb_fielddef_msgsubdef(f); - const upb_fielddef* key_f = upb_msgdef_field(entry_m, 0); - const upb_fielddef* val_f = upb_msgdef_field(entry_m, 1); - upb_arena* arena = PyUpb_Arena_Get(self->arena); - upb_msgval u_key, u_val; - if (!PyUpb_PyToUpb(key, key_f, &u_key, arena)) return NULL; - if (!map || !upb_map_get(map, u_key, &u_val)) { - map = PyUpb_MapContainer_AssureWritable(self); - upb_arena* arena = PyUpb_Arena_Get(self->arena); - if (upb_fielddef_issubmsg(val_f)) { - u_val.msg_val = upb_msg_new(upb_fielddef_msgsubdef(val_f), arena); - } else { - memset(&u_val, 0, sizeof(u_val)); - } - upb_map_set(map, u_key, u_val, arena); - } - return PyUpb_UpbToPy(u_val, val_f, self->arena); -} - PyObject* PyUpb_MapContainer_GetOrCreateWrapper(upb_map* map, const upb_fielddef* f, PyObject* arena) { - PyObject* ret = PyUpb_ObjCache_Get(map); - - if (!ret) { - PyTypeObject* cls = PyUpb_MapContainer_GetClass(f); - PyUpb_MapContainer* map = (void*)PyType_GenericAlloc(cls, 0); - map->arena = arena; - map->field = (uintptr_t)f; - map->ptr.map = map; - map->version = 0; - ret = &map->ob_base; - Py_INCREF(arena); - PyUpb_ObjCache_Add(map, ret); - } + PyUpb_MapContainer* ret = (void*)PyUpb_ObjCache_Get(map); + if (ret) return &ret->ob_base; - return ret; + PyTypeObject* cls = PyUpb_MapContainer_GetClass(f); + ret = (void*)PyType_GenericAlloc(cls, 0); + ret->arena = arena; + ret->field = (uintptr_t)f; + ret->ptr.map = map; + ret->version = 0; + Py_INCREF(arena); + PyUpb_ObjCache_Add(map, &ret->ob_base); + return &ret->ob_base; } // ----------------------------------------------------------------------------- diff --git a/python/protobuf.c b/python/protobuf.c index ccec6b286c..55d46aa86c 100644 --- a/python/protobuf.c +++ b/python/protobuf.c @@ -253,18 +253,24 @@ static const char *PyUpb_GetClassName(PyType_Spec *spec) { } PyTypeObject *PyUpb_AddClass(PyObject *m, PyType_Spec *spec) { - PyObject *type = PyType_FromSpec(spec); + PyObject *type = (void*)PyType_FromSpec(spec); const char *name = PyUpb_GetClassName(spec); - int result = PyModule_AddObject(m, name, type); - return type && result == 0 ? (PyTypeObject*)type : NULL; + if (PyModule_AddObject(m, name, type) < 0) { + Py_XDECREF(type); + return NULL; + } + return (PyTypeObject*)type; } PyTypeObject* PyUpb_AddClassWithBases(PyObject* m, PyType_Spec* spec, PyObject* bases) { PyObject* type = PyType_FromSpecWithBases(spec, bases); const char* name = PyUpb_GetClassName(spec); - int result = PyModule_AddObject(m, name, type); - return type && result == 0 ? (PyTypeObject*)type : NULL; + if (PyModule_AddObject(m, name, type) < 0) { + Py_XDECREF(type); + return NULL; + } + return (PyTypeObject*)type; } const char *PyUpb_GetStrData(PyObject *obj) { diff --git a/upb/msg.c b/upb/msg.c index 6667acbb8a..1e7785b39c 100644 --- a/upb/msg.c +++ b/upb/msg.c @@ -285,7 +285,6 @@ static int _upb_mapsorter_cmpstr(const void *_a, const void *_b) { return a.size - b.size; } -#include bool _upb_mapsorter_pushmap(_upb_mapsorter *s, upb_descriptortype_t key_type, const upb_map *map, _upb_sortedmap *sorted) { int map_size = _upb_map_size(map);