Addressed PR comments.

pull/13171/head
Joshua Haberman 3 years ago
parent 54b775d4f4
commit e5d8d28b0f
  1. 4
      python/map.c
  2. 1
      python/map.h
  3. 34
      python/message.c
  4. 16
      python/minimal_test.py
  5. 5
      python/repeated.c
  6. 1
      python/repeated.h
  7. 1
      upb/reflection.c

@ -89,6 +89,9 @@ PyTypeObject* PyUpb_MapContainer_GetClass(const upb_fielddef* f) {
PyObject* PyUpb_MapContainer_NewStub(PyObject* parent, const upb_fielddef* f,
PyObject* arena) {
// We only create stubs when the parent is reified, by convention. However
// this is not an invariant: the parent could become reified at any time.
assert(PyUpb_CMessage_GetIfReified(parent) == NULL);
PyTypeObject* cls = PyUpb_MapContainer_GetClass(f);
PyUpb_MapContainer* map = (void*)PyType_GenericAlloc(cls, 0);
map->arena = arena;
@ -196,7 +199,6 @@ 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_EnsureReified(_self);
upb_map_clear(map);
Py_RETURN_NONE;

@ -34,6 +34,7 @@
#include "upb/def.h"
// Creates a new repeated field stub for field `f` of message object `parent`.
// Precondition: `parent` must be a stub.
PyObject* PyUpb_MapContainer_NewStub(PyObject* parent, const upb_fielddef* f,
PyObject* arena);

@ -715,12 +715,30 @@ PyObject* PyUpb_CMessage_Get(upb_msg* u_msg, const upb_msgdef* m,
return ret;
}
PyObject* PyUpb_CMessage_GetUnsetWrapper(PyUpb_CMessage* self,
const upb_fielddef* field) {
/* PyUpb_CMessage_GetStub()
*
* Non-present messages return "stub" objects that point to their parent, but
* will materialize into real upb objects if they are mutated.
*
* Note: we do *not* create stubs for repeated/map fields unless the parent
* is a stub:
*
* msg = TestMessage()
* msg.submessage # (A) Creates a stub
* msg.repeated_foo # (B) Does *not* create a stub
* msg.submessage.repeated_bar # (C) Creates a stub
*
* In case (B) we have some freedom: we could either create a stub, or create
* a reified object with underlying data. It appears that either could work
* equally well, with no observable change to users. There isn't a clear
* advantage to either choice. We choose to follow the behavior of the
* pre-existing C++ behavior for consistency, but if it becomes apparent that
* there would be some benefit to reversing this decision, it should be totally
* within the realm of possibility.
*/
PyObject* PyUpb_CMessage_GetStub(PyUpb_CMessage* self,
const upb_fielddef* field) {
PyObject* _self = (void*)self;
// Non-present messages return magical "empty" messages that point to their
// parent, but will materialize into real messages if any fields are
// assigned.
if (!self->unset_subobj_map) {
self->unset_subobj_map = PyUpb_WeakMap_New();
}
@ -786,8 +804,8 @@ PyObject* PyUpb_CMessage_GetFieldValue(PyObject* _self,
bool seq = upb_fielddef_isseq(field);
if ((PyUpb_CMessage_IsStub(self) && (submsg || seq)) ||
(submsg && !upb_msg_has(self->ptr.msg, field))) {
return PyUpb_CMessage_GetUnsetWrapper(self, field);
(submsg && !seq && !upb_msg_has(self->ptr.msg, field))) {
return PyUpb_CMessage_GetStub(self, field);
} else if (seq) {
return PyUpb_CMessage_GetPresentWrapper(self, field);
} else {
@ -1053,7 +1071,7 @@ void PyUpb_CMessage_DoClearField(PyObject* _self, const upb_fielddef* f) {
PyUpb_MapContainer_Invalidate(sub);
} else if (upb_fielddef_isseq(f)) {
if (sub) {
//PyUpb_RepeatedContainer_EnsureReified(sub);
PyUpb_RepeatedContainer_EnsureReified(sub);
}
} else if (upb_fielddef_issubmsg(f)) {
if (sub) {

@ -82,11 +82,11 @@ class TestMessageExtension(unittest.TestCase):
self.assertRaises(AttributeError, getattr, msg, 'Extensions')
def testClearStubMapField(self):
msg = map_unittest_pb2.TestMap()
int32_map = msg.map_int32_int32
msg.ClearField("map_int32_int32")
msg = map_unittest_pb2.TestMapSubmessage()
int32_map = msg.test_map.map_int32_int32
msg.test_map.ClearField("map_int32_int32")
int32_map[123] = 456
self.assertEqual(0, msg.ByteSize())
self.assertEqual(0, msg.test_map.ByteSize())
def testClearReifiedMapField(self):
msg = map_unittest_pb2.TestMap()
@ -97,11 +97,11 @@ class TestMessageExtension(unittest.TestCase):
self.assertEqual(0, msg.ByteSize())
def testClearStubRepeatedField(self):
msg = unittest_pb2.TestAllTypes()
int32_array = msg.repeated_int32
msg.ClearField("repeated_int32")
msg = unittest_pb2.NestedTestAllTypes()
int32_array = msg.payload.repeated_int32
msg.payload.ClearField("repeated_int32")
int32_array.append(123)
self.assertEqual(0, msg.ByteSize())
self.assertEqual(0, msg.payload.ByteSize())
def testClearReifiedRepeatdField(self):
msg = unittest_pb2.TestAllTypes()

@ -160,6 +160,9 @@ static Py_ssize_t PyUpb_RepeatedContainer_Length(PyObject* self) {
PyObject* PyUpb_RepeatedContainer_NewStub(PyObject* parent,
const upb_fielddef* f,
PyObject* arena) {
// We only create stubs when the parent is reified, by convention. However
// this is not an invariant: the parent could become reified at any time.
assert(PyUpb_CMessage_GetIfReified(parent) == NULL);
PyTypeObject* cls = PyUpb_RepeatedContainer_GetClass(f);
PyUpb_RepeatedContainer* repeated = (void*)PyType_GenericAlloc(cls, 0);
repeated->arena = arena;
@ -193,8 +196,8 @@ static PyObject* PyUpb_RepeatedContainer_MergeFrom(PyObject* _self,
PyObject* PyUpb_RepeatedContainer_DeepCopy(PyObject* _self, PyObject* value) {
PyUpb_RepeatedContainer* self = (PyUpb_RepeatedContainer*)_self;
PyUpb_RepeatedContainer* clone = (void*)PyType_GenericAlloc(Py_TYPE(_self), 0);
const upb_fielddef* f = PyUpb_RepeatedContainer_GetField(self);
if (clone == NULL) return NULL;
const upb_fielddef* f = PyUpb_RepeatedContainer_GetField(self);
clone->arena = PyUpb_Arena_New();
clone->field = (uintptr_t)PyUpb_FieldDescriptor_Get(f);
clone->ptr.arr =

@ -34,6 +34,7 @@
#include "upb/def.h"
// Creates a new repeated field stub for field `f` of message object `parent`.
// Precondition: `parent` must be a stub.
PyObject* PyUpb_RepeatedContainer_NewStub(PyObject* parent,
const upb_fielddef* f,
PyObject* arena);

@ -108,6 +108,7 @@ static upb_msgval _upb_msg_getraw(const upb_msg *msg, const upb_fielddef *f) {
}
bool upb_msg_has(const upb_msg *msg, const upb_fielddef *f) {
assert(upb_fielddef_haspresence(f));
if (upb_fielddef_isextension(f)) {
const upb_msglayout_ext *ext = _upb_fielddef_extlayout(f);
return _upb_msg_getext(msg, ext) != NULL;

Loading…
Cancel
Save