Addressed PR comments.
diff --git a/python/map.c b/python/map.c
index c7f5675..0477c59 100644
--- a/python/map.c
+++ b/python/map.c
@@ -89,6 +89,9 @@
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_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;
diff --git a/python/map.h b/python/map.h
index 8430f3b..46ae75d 100644
--- a/python/map.h
+++ b/python/map.h
@@ -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);
diff --git a/python/message.c b/python/message.c
index a3da671..81d5381 100644
--- a/python/message.c
+++ b/python/message.c
@@ -715,12 +715,30 @@
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 @@
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 @@
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) {
diff --git a/python/minimal_test.py b/python/minimal_test.py
index a87f6b3..a20c462 100644
--- a/python/minimal_test.py
+++ b/python/minimal_test.py
@@ -82,11 +82,11 @@
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 @@
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()
diff --git a/python/repeated.c b/python/repeated.c
index 09e5657..ca9182c 100644
--- a/python/repeated.c
+++ b/python/repeated.c
@@ -160,6 +160,9 @@
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 @@
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 =
diff --git a/python/repeated.h b/python/repeated.h
index 9e8530b..8204afd 100644
--- a/python/repeated.h
+++ b/python/repeated.h
@@ -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);
