From 00700b72191d620402d5eb0390b5460b35c93e05 Mon Sep 17 00:00:00 2001 From: Josh Haberman Date: Tue, 6 Oct 2015 14:13:09 -0700 Subject: Removed all warnings from the Python/C++ build. Also made the Travis build ensure that no warnings are present. These builds were previously spewing many warnings, which was hiding warnings for important things like accidentally using C++11-only features. Change-Id: I56caeee9db48bc78756a3e8d7c14874630627037 --- python/google/protobuf/pyext/descriptor.cc | 7 +- .../google/protobuf/pyext/descriptor_containers.cc | 6 +- python/google/protobuf/pyext/descriptor_pool.cc | 2 +- python/google/protobuf/pyext/extension_dict.cc | 2 +- python/google/protobuf/pyext/message.cc | 112 ++++++++++----------- .../google/protobuf/pyext/message_map_container.cc | 26 ++--- .../protobuf/pyext/repeated_composite_container.cc | 23 +++-- .../protobuf/pyext/repeated_composite_container.h | 3 - .../protobuf/pyext/repeated_scalar_container.cc | 46 ++++----- .../google/protobuf/pyext/scalar_map_container.cc | 22 ++-- python/google/protobuf/pyext/scoped_pyobject_ptr.h | 7 -- python/setup.py | 9 +- python/tox.ini | 2 +- 13 files changed, 133 insertions(+), 134 deletions(-) diff --git a/python/google/protobuf/pyext/descriptor.cc b/python/google/protobuf/pyext/descriptor.cc index b238fd02..61a3d237 100644 --- a/python/google/protobuf/pyext/descriptor.cc +++ b/python/google/protobuf/pyext/descriptor.cc @@ -232,7 +232,7 @@ static PyObject* GetOrBuildOptions(const DescriptorClass *descriptor) { } // Cache the result. - Py_INCREF(value); + Py_INCREF(value.get()); (*pool->descriptor_options)[descriptor] = value.get(); return value.release(); @@ -1489,7 +1489,8 @@ static bool AddEnumValues(PyTypeObject *type, if (obj == NULL) { return false; } - if (PyDict_SetItemString(type->tp_dict, value->name().c_str(), obj) < 0) { + if (PyDict_SetItemString(type->tp_dict, value->name().c_str(), obj.get()) < + 0) { return false; } } @@ -1498,7 +1499,7 @@ static bool AddEnumValues(PyTypeObject *type, static bool AddIntConstant(PyTypeObject *type, const char* name, int value) { ScopedPyObjectPtr obj(PyInt_FromLong(value)); - if (PyDict_SetItemString(type->tp_dict, name, obj) < 0) { + if (PyDict_SetItemString(type->tp_dict, name, obj.get()) < 0) { return false; } return true; diff --git a/python/google/protobuf/pyext/descriptor_containers.cc b/python/google/protobuf/pyext/descriptor_containers.cc index b20f5e4f..e505d812 100644 --- a/python/google/protobuf/pyext/descriptor_containers.cc +++ b/python/google/protobuf/pyext/descriptor_containers.cc @@ -355,7 +355,7 @@ static int DescriptorSequence_Equal(PyContainer* self, PyObject* other) { if (value2 == NULL) { return -1; } - int cmp = PyObject_RichCompareBool(value1, value2, Py_EQ); + int cmp = PyObject_RichCompareBool(value1.get(), value2, Py_EQ); if (cmp != 1) // error or not equal return cmp; } @@ -399,12 +399,12 @@ static int DescriptorMapping_Equal(PyContainer* self, PyObject* other) { if (value1 == NULL) { return -1; } - PyObject* value2 = PyDict_GetItem(other, key); + PyObject* value2 = PyDict_GetItem(other, key.get()); if (value2 == NULL) { // Not found in the other dictionary return 0; } - int cmp = PyObject_RichCompareBool(value1, value2, Py_EQ); + int cmp = PyObject_RichCompareBool(value1.get(), value2, Py_EQ); if (cmp != 1) // error or not equal return cmp; } diff --git a/python/google/protobuf/pyext/descriptor_pool.cc b/python/google/protobuf/pyext/descriptor_pool.cc index 6443a7d5..0f7487fa 100644 --- a/python/google/protobuf/pyext/descriptor_pool.cc +++ b/python/google/protobuf/pyext/descriptor_pool.cc @@ -394,7 +394,7 @@ PyObject* Add(PyDescriptorPool* self, PyObject* file_descriptor_proto) { if (serialized_pb == NULL) { return NULL; } - return AddSerializedFile(self, serialized_pb); + return AddSerializedFile(self, serialized_pb.get()); } static PyMethodDef Methods[] = { diff --git a/python/google/protobuf/pyext/extension_dict.cc b/python/google/protobuf/pyext/extension_dict.cc index 9c9b4178..b361b342 100644 --- a/python/google/protobuf/pyext/extension_dict.cc +++ b/python/google/protobuf/pyext/extension_dict.cc @@ -211,7 +211,7 @@ PyObject* _FindExtensionByName(ExtensionDict* self, PyObject* name) { if (extensions_by_name == NULL) { return NULL; } - PyObject* result = PyDict_GetItem(extensions_by_name, name); + PyObject* result = PyDict_GetItem(extensions_by_name.get(), name); if (result == NULL) { Py_RETURN_NONE; } else { diff --git a/python/google/protobuf/pyext/message.cc b/python/google/protobuf/pyext/message.cc index 9318c834..db58df54 100644 --- a/python/google/protobuf/pyext/message.cc +++ b/python/google/protobuf/pyext/message.cc @@ -142,7 +142,7 @@ static bool AddFieldNumberToClass( if (number == NULL) { return false; } - if (PyObject_SetAttr(cls, attr_name, number) == -1) { + if (PyObject_SetAttr(cls, attr_name.get(), number.get()) == -1) { return false; } return true; @@ -155,11 +155,11 @@ static int AddDescriptors(PyObject* cls, const Descriptor* descriptor) { // classes will register themselves in this class. if (descriptor->extension_range_count() > 0) { ScopedPyObjectPtr by_name(PyDict_New()); - if (PyObject_SetAttr(cls, k_extensions_by_name, by_name) < 0) { + if (PyObject_SetAttr(cls, k_extensions_by_name, by_name.get()) < 0) { return -1; } ScopedPyObjectPtr by_number(PyDict_New()); - if (PyObject_SetAttr(cls, k_extensions_by_number, by_number) < 0) { + if (PyObject_SetAttr(cls, k_extensions_by_number, by_number.get()) < 0) { return -1; } } @@ -190,7 +190,7 @@ static int AddDescriptors(PyObject* cls, const Descriptor* descriptor) { return -1; } if (PyObject_SetAttrString( - cls, enum_descriptor->name().c_str(), wrapped) == -1) { + cls, enum_descriptor->name().c_str(), wrapped.get()) == -1) { return -1; } @@ -203,8 +203,8 @@ static int AddDescriptors(PyObject* cls, const Descriptor* descriptor) { if (value_number == NULL) { return -1; } - if (PyObject_SetAttrString( - cls, enum_value_descriptor->name().c_str(), value_number) == -1) { + if (PyObject_SetAttrString(cls, enum_value_descriptor->name().c_str(), + value_number.get()) == -1) { return -1; } } @@ -224,7 +224,7 @@ static int AddDescriptors(PyObject* cls, const Descriptor* descriptor) { // Add the extension field to the message class. if (PyObject_SetAttrString( - cls, field->name().c_str(), extension_field) == -1) { + cls, field->name().c_str(), extension_field.get()) == -1) { return -1; } @@ -280,7 +280,7 @@ static PyObject* New(PyTypeObject* type, return NULL; } // Call the base metaclass. - ScopedPyObjectPtr result(PyType_Type.tp_new(type, new_args, NULL)); + ScopedPyObjectPtr result(PyType_Type.tp_new(type, new_args.get(), NULL)); if (result == NULL) { return NULL; } @@ -318,7 +318,7 @@ static PyObject* New(PyTypeObject* type, } // Continue with type initialization: add other descriptors, enum values... - if (AddDescriptors(result, descriptor) < 0) { + if (AddDescriptors(result.get(), descriptor) < 0) { return NULL; } return result.release(); @@ -330,11 +330,6 @@ static void Dealloc(PyMessageMeta *self) { Py_TYPE(self)->tp_free(reinterpret_cast(self)); } -static PyObject* GetDescriptor(PyMessageMeta *self, void *closure) { - Py_INCREF(self->py_message_descriptor); - return self->py_message_descriptor; -} - // This function inserts and empty weakref at the end of the list of // subclasses for the main protocol buffer Message class. @@ -862,7 +857,6 @@ int AssureWritable(CMessage* self) { return -1; // Make self->message writable. - Message* parent_message = self->parent->message; Message* mutable_message = GetMutableMessage( self->parent, self->parent_field_descriptor); @@ -1106,8 +1100,8 @@ int InitAttributes(CMessage* self, PyObject* kwargs) { return -1; } ScopedPyObjectPtr next; - while ((next.reset(PyIter_Next(iter))) != NULL) { - PyObject* kwargs = (PyDict_Check(next) ? next.get() : NULL); + while ((next.reset(PyIter_Next(iter.get()))) != NULL) { + PyObject* kwargs = (PyDict_Check(next.get()) ? next.get() : NULL); ScopedPyObjectPtr new_msg( repeated_composite_container::Add(rc_container, NULL, kwargs)); if (new_msg == NULL) { @@ -1115,9 +1109,9 @@ int InitAttributes(CMessage* self, PyObject* kwargs) { } if (kwargs == NULL) { // next was not a dict, it's a message we need to merge - ScopedPyObjectPtr merged( - MergeFrom(reinterpret_cast(new_msg.get()), next)); - if (merged == NULL) { + ScopedPyObjectPtr merged(MergeFrom( + reinterpret_cast(new_msg.get()), next.get())); + if (merged.get() == NULL) { return -1; } } @@ -1135,13 +1129,14 @@ int InitAttributes(CMessage* self, PyObject* kwargs) { return -1; } ScopedPyObjectPtr next; - while ((next.reset(PyIter_Next(iter))) != NULL) { - ScopedPyObjectPtr enum_value(GetIntegerEnumValue(*descriptor, next)); + while ((next.reset(PyIter_Next(iter.get()))) != NULL) { + ScopedPyObjectPtr enum_value( + GetIntegerEnumValue(*descriptor, next.get())); if (enum_value == NULL) { return -1; } - ScopedPyObjectPtr new_msg( - repeated_scalar_container::Append(rs_container, enum_value)); + ScopedPyObjectPtr new_msg(repeated_scalar_container::Append( + rs_container, enum_value.get())); if (new_msg == NULL) { return -1; } @@ -1182,7 +1177,8 @@ int InitAttributes(CMessage* self, PyObject* kwargs) { return -1; } } - if (SetAttr(self, name, (new_val == NULL) ? value : new_val) < 0) { + if (SetAttr(self, name, (new_val.get() == NULL) ? value : new_val.get()) < + 0) { return -1; } } @@ -1769,13 +1765,13 @@ static PyObject* SerializeToString(CMessage* self, PyObject* args) { } ScopedPyObjectPtr encode_error( - PyObject_GetAttrString(message_module, "EncodeError")); + PyObject_GetAttrString(message_module.get(), "EncodeError")); if (encode_error.get() == NULL) { return NULL; } PyErr_Format(encode_error.get(), "Message %s is missing required fields: %s", - GetMessageName(self).c_str(), PyString_AsString(joined)); + GetMessageName(self).c_str(), PyString_AsString(joined.get())); return NULL; } int size = self->message->ByteSize(); @@ -1965,7 +1961,8 @@ static PyObject* RegisterExtension(PyObject* cls, } // If the extension was already registered, check that it is the same. - PyObject* existing_extension = PyDict_GetItem(extensions_by_name, full_name); + PyObject* existing_extension = + PyDict_GetItem(extensions_by_name.get(), full_name.get()); if (existing_extension != NULL) { const FieldDescriptor* existing_extension_descriptor = GetExtensionDescriptor(existing_extension); @@ -1977,7 +1974,8 @@ static PyObject* RegisterExtension(PyObject* cls, Py_RETURN_NONE; } - if (PyDict_SetItem(extensions_by_name, full_name, extension_handle) < 0) { + if (PyDict_SetItem(extensions_by_name.get(), full_name.get(), + extension_handle) < 0) { return NULL; } @@ -1992,7 +1990,8 @@ static PyObject* RegisterExtension(PyObject* cls, if (number == NULL) { return NULL; } - if (PyDict_SetItem(extensions_by_number, number, extension_handle) < 0) { + if (PyDict_SetItem(extensions_by_number.get(), number.get(), + extension_handle) < 0) { return NULL; } @@ -2008,7 +2007,8 @@ static PyObject* RegisterExtension(PyObject* cls, if (message_name == NULL) { return NULL; } - PyDict_SetItem(extensions_by_name, message_name, extension_handle); + PyDict_SetItem(extensions_by_name.get(), message_name.get(), + extension_handle); } Py_RETURN_NONE; @@ -2058,7 +2058,7 @@ static PyObject* ListFields(CMessage* self) { // the field information. Thus the actual size of the py list will be // smaller than the size of fields. Set the actual size at the end. Py_ssize_t actual_size = 0; - for (Py_ssize_t i = 0; i < fields.size(); ++i) { + for (size_t i = 0; i < fields.size(); ++i) { ScopedPyObjectPtr t(PyTuple_New(2)); if (t == NULL) { return NULL; @@ -2086,7 +2086,7 @@ static PyObject* ListFields(CMessage* self) { return NULL; } // 'extension' reference later stolen by PyTuple_SET_ITEM. - PyObject* extension = PyObject_GetItem(extensions, extension_field); + PyObject* extension = PyObject_GetItem(extensions, extension_field.get()); if (extension == NULL) { return NULL; } @@ -2108,9 +2108,9 @@ static PyObject* ListFields(CMessage* self) { return NULL; } - PyObject* field_value = GetAttr(self, py_field_name); + PyObject* field_value = GetAttr(self, py_field_name.get()); if (field_value == NULL) { - PyErr_SetObject(PyExc_ValueError, py_field_name); + PyErr_SetObject(PyExc_ValueError, py_field_name.get()); return NULL; } PyTuple_SET_ITEM(t.get(), 0, field_descriptor.release()); @@ -2132,7 +2132,7 @@ PyObject* FindInitializationErrors(CMessage* self) { if (error_list == NULL) { return NULL; } - for (Py_ssize_t i = 0; i < errors.size(); ++i) { + for (size_t i = 0; i < errors.size(); ++i) { const string& error = errors[i]; PyObject* error_string = PyString_FromStringAndSize( error.c_str(), error.length()); @@ -2430,16 +2430,16 @@ PyObject* ToUnicode(CMessage* self) { return NULL; } Py_INCREF(Py_True); - ScopedPyObjectPtr encoded(PyObject_CallMethodObjArgs(text_format, method_name, - self, Py_True, NULL)); + ScopedPyObjectPtr encoded(PyObject_CallMethodObjArgs( + text_format.get(), method_name.get(), self, Py_True, NULL)); Py_DECREF(Py_True); if (encoded == NULL) { return NULL; } #if PY_MAJOR_VERSION < 3 - PyObject* decoded = PyString_AsDecodedObject(encoded, "utf-8", NULL); + PyObject* decoded = PyString_AsDecodedObject(encoded.get(), "utf-8", NULL); #else - PyObject* decoded = PyUnicode_FromEncodedObject(encoded, "utf-8", NULL); + PyObject* decoded = PyUnicode_FromEncodedObject(encoded.get(), "utf-8", NULL); #endif if (decoded == NULL) { return NULL; @@ -2462,7 +2462,7 @@ PyObject* Reduce(CMessage* self) { if (serialized == NULL) { return NULL; } - if (PyDict_SetItemString(state, "serialized", serialized) < 0) { + if (PyDict_SetItemString(state.get(), "serialized", serialized.get()) < 0) { return NULL; } return Py_BuildValue("OOO", constructor.get(), args.get(), state.get()); @@ -2817,16 +2817,16 @@ bool InitProto2MessageModule(PyObject *m) { if (empty_dict == NULL) { return false; } - ScopedPyObjectPtr immutable_dict(PyDictProxy_New(empty_dict)); + ScopedPyObjectPtr immutable_dict(PyDictProxy_New(empty_dict.get())); if (immutable_dict == NULL) { return false; } if (PyDict_SetItem(CMessage_Type.tp_dict, - k_extensions_by_name, immutable_dict) < 0) { + k_extensions_by_name, immutable_dict.get()) < 0) { return false; } if (PyDict_SetItem(CMessage_Type.tp_dict, - k_extensions_by_number, immutable_dict) < 0) { + k_extensions_by_number, immutable_dict.get()) < 0) { return false; } @@ -2856,19 +2856,19 @@ bool InitProto2MessageModule(PyObject *m) { if (collections == NULL) { return false; } - ScopedPyObjectPtr mutable_sequence(PyObject_GetAttrString( - collections, "MutableSequence")); + ScopedPyObjectPtr mutable_sequence( + PyObject_GetAttrString(collections.get(), "MutableSequence")); if (mutable_sequence == NULL) { return false; } - if (ScopedPyObjectPtr(PyObject_CallMethod(mutable_sequence, "register", "O", - &RepeatedScalarContainer_Type)) - == NULL) { + if (ScopedPyObjectPtr( + PyObject_CallMethod(mutable_sequence.get(), "register", "O", + &RepeatedScalarContainer_Type)) == NULL) { return false; } - if (ScopedPyObjectPtr(PyObject_CallMethod(mutable_sequence, "register", "O", - &RepeatedCompositeContainer_Type)) - == NULL) { + if (ScopedPyObjectPtr( + PyObject_CallMethod(mutable_sequence.get(), "register", "O", + &RepeatedCompositeContainer_Type)) == NULL) { return false; } } @@ -2883,16 +2883,16 @@ bool InitProto2MessageModule(PyObject *m) { } ScopedPyObjectPtr mutable_mapping( - PyObject_GetAttrString(containers, "MutableMapping")); + PyObject_GetAttrString(containers.get(), "MutableMapping")); if (mutable_mapping == NULL) { return false; } - if (!PyObject_TypeCheck(mutable_mapping, &PyType_Type)) { + if (!PyObject_TypeCheck(mutable_mapping.get(), &PyType_Type)) { return false; } - Py_INCREF(mutable_mapping); + Py_INCREF(mutable_mapping.get()); #if PY_MAJOR_VERSION >= 3 PyObject* bases = PyTuple_New(1); PyTuple_SET_ITEM(bases, 0, mutable_mapping.get()); @@ -2925,7 +2925,7 @@ bool InitProto2MessageModule(PyObject *m) { PyType_FromSpecWithBases(&MessageMapContainer_Type_spec, bases); PyModule_AddObject(m, "MessageMapContainer", MessageMapContainer_Type); #else - Py_INCREF(mutable_mapping); + Py_INCREF(mutable_mapping.get()); MessageMapContainer_Type.tp_base = reinterpret_cast(mutable_mapping.get()); diff --git a/python/google/protobuf/pyext/message_map_container.cc b/python/google/protobuf/pyext/message_map_container.cc index f54d2015..8902fa00 100644 --- a/python/google/protobuf/pyext/message_map_container.cc +++ b/python/google/protobuf/pyext/message_map_container.cc @@ -155,7 +155,7 @@ static PyObject* GetCMessage(MessageMapContainer* self, Message* entry) { Message* message = entry->GetReflection()->MutableMessage( entry, self->value_field_descriptor); ScopedPyObjectPtr key(PyLong_FromVoidPtr(message)); - PyObject* ret = PyDict_GetItem(self->message_dict, key); + PyObject* ret = PyDict_GetItem(self->message_dict, key.get()); if (ret == NULL) { CMessage* cmsg = cmessage::NewEmptyMessage(self->subclass_init, @@ -169,7 +169,7 @@ static PyObject* GetCMessage(MessageMapContainer* self, Message* entry) { cmsg->message = message; cmsg->parent = self->parent; - if (PyDict_SetItem(self->message_dict, key, ret) < 0) { + if (PyDict_SetItem(self->message_dict, key.get(), ret) < 0) { Py_DECREF(ret); return NULL; } @@ -202,7 +202,7 @@ int MapKeyMatches(MessageMapContainer* self, const Message* entry, // TODO(haberman): do we need more strict type checking? ScopedPyObjectPtr entry_key( cmessage::InternalGetScalar(entry, self->key_field_descriptor)); - int ret = PyObject_RichCompareBool(key, entry_key, Py_EQ); + int ret = PyObject_RichCompareBool(key, entry_key.get(), Py_EQ); return ret; } @@ -237,7 +237,7 @@ int SetItem(PyObject *_self, PyObject *key, PyObject *v) { if (matches < 0) return -1; if (matches) { found = true; - if (i != size - 1) { + if (i != (int)size - 1) { reflection->SwapElements(message, self->parent_field_descriptor, i, size - 1); } @@ -266,7 +266,7 @@ PyObject* GetIterator(PyObject *_self) { return PyErr_Format(PyExc_KeyError, "Could not allocate iterator"); } - MessageMapIterator* iter = GetIter(obj); + MessageMapIterator* iter = GetIter(obj.get()); Py_INCREF(self); iter->container = self; @@ -354,7 +354,7 @@ PyObject* Contains(PyObject* _self, PyObject* key) { // via linear search. // // TODO(haberman): add lookup API to Reflection API. - size_t size = + int size = reflection->FieldSize(*message, self->parent_field_descriptor); for (int i = 0; i < size; i++) { Message* entry = reflection->MutableRepeatedMessage( @@ -405,12 +405,6 @@ PyObject* Get(PyObject* self, PyObject* args) { } } -static PyMappingMethods MpMethods = { - Length, // mp_length - GetItem, // mp_subscript - SetItem, // mp_ass_subscript -}; - static void Dealloc(PyObject* _self) { MessageMapContainer* self = GetMap(_self); self->owner.reset(); @@ -485,6 +479,12 @@ PyObject* IterNext(PyObject* _self) { PyObject *MessageMapContainer_Type; #else + static PyMappingMethods MpMethods = { + message_map_container::Length, // mp_length + message_map_container::GetItem, // mp_subscript + message_map_container::SetItem, // mp_ass_subscript + }; + PyTypeObject MessageMapContainer_Type = { PyVarObject_HEAD_INIT(&PyType_Type, 0) FULL_MODULE_NAME ".MessageMapContainer", // tp_name @@ -498,7 +498,7 @@ PyObject* IterNext(PyObject* _self) { 0, // tp_repr 0, // tp_as_number 0, // tp_as_sequence - &message_map_container::MpMethods, // tp_as_mapping + &MpMethods, // tp_as_mapping 0, // tp_hash 0, // tp_call 0, // tp_str diff --git a/python/google/protobuf/pyext/repeated_composite_container.cc b/python/google/protobuf/pyext/repeated_composite_container.cc index fe2e600b..b01123b4 100644 --- a/python/google/protobuf/pyext/repeated_composite_container.cc +++ b/python/google/protobuf/pyext/repeated_composite_container.cc @@ -116,7 +116,7 @@ static int UpdateChildMessages(RepeatedCompositeContainer* self) { cmsg->owner = self->owner; cmsg->message = const_cast(&sub_message); cmsg->parent = self->parent; - if (PyList_Append(self->child_messages, py_cmsg) < 0) { + if (PyList_Append(self->child_messages, py_cmsg.get()) < 0) { return -1; } } @@ -202,8 +202,8 @@ PyObject* Extend(RepeatedCompositeContainer* self, PyObject* value) { return NULL; } ScopedPyObjectPtr next; - while ((next.reset(PyIter_Next(iter))) != NULL) { - if (!PyObject_TypeCheck(next, &CMessage_Type)) { + while ((next.reset(PyIter_Next(iter.get()))) != NULL) { + if (!PyObject_TypeCheck(next.get(), &CMessage_Type)) { PyErr_SetString(PyExc_TypeError, "Not a cmessage"); return NULL; } @@ -212,7 +212,8 @@ PyObject* Extend(RepeatedCompositeContainer* self, PyObject* value) { return NULL; } CMessage* new_cmessage = reinterpret_cast(new_message.get()); - if (ScopedPyObjectPtr(cmessage::MergeFrom(new_cmessage, next)) == NULL) { + if (ScopedPyObjectPtr(cmessage::MergeFrom(new_cmessage, next.get())) == + NULL) { return NULL; } } @@ -294,7 +295,7 @@ static PyObject* Remove(RepeatedCompositeContainer* self, PyObject* value) { return NULL; } ScopedPyObjectPtr py_index(PyLong_FromLong(index)); - if (AssignSubscript(self, py_index, NULL) < 0) { + if (AssignSubscript(self, py_index.get(), NULL) < 0) { return NULL; } Py_RETURN_NONE; @@ -318,17 +319,17 @@ static PyObject* RichCompare(RepeatedCompositeContainer* self, if (full_slice == NULL) { return NULL; } - ScopedPyObjectPtr list(Subscript(self, full_slice)); + ScopedPyObjectPtr list(Subscript(self, full_slice.get())); if (list == NULL) { return NULL; } ScopedPyObjectPtr other_list( - Subscript( - reinterpret_cast(other), full_slice)); + Subscript(reinterpret_cast(other), + full_slice.get())); if (other_list == NULL) { return NULL; } - return PyObject_RichCompare(list, other_list, opid); + return PyObject_RichCompare(list.get(), other_list.get(), opid); } else { Py_INCREF(Py_NotImplemented); return Py_NotImplemented; @@ -365,7 +366,7 @@ static int SortPythonMessages(RepeatedCompositeContainer* self, ScopedPyObjectPtr m(PyObject_GetAttrString(self->child_messages, "sort")); if (m == NULL) return -1; - if (PyObject_Call(m, args, kwds) == NULL) + if (PyObject_Call(m.get(), args, kwds) == NULL) return -1; if (self->message != NULL) { ReorderAttached(self); @@ -429,7 +430,7 @@ static PyObject* Pop(RepeatedCompositeContainer* self, return NULL; } ScopedPyObjectPtr py_index(PyLong_FromSsize_t(index)); - if (AssignSubscript(self, py_index, NULL) < 0) { + if (AssignSubscript(self, py_index.get(), NULL) < 0) { return NULL; } return item; diff --git a/python/google/protobuf/pyext/repeated_composite_container.h b/python/google/protobuf/pyext/repeated_composite_container.h index e0f21360..3013aba9 100644 --- a/python/google/protobuf/pyext/repeated_composite_container.h +++ b/python/google/protobuf/pyext/repeated_composite_container.h @@ -108,9 +108,6 @@ PyObject *NewContainer( const FieldDescriptor* parent_field_descriptor, PyObject *concrete_class); -// Returns the number of items in this repeated composite container. -static Py_ssize_t Length(RepeatedCompositeContainer* self); - // Appends a new CMessage to the container and returns it. The // CMessage is initialized using the content of kwargs. // diff --git a/python/google/protobuf/pyext/repeated_scalar_container.cc b/python/google/protobuf/pyext/repeated_scalar_container.cc index 7565c6fd..95da85f8 100644 --- a/python/google/protobuf/pyext/repeated_scalar_container.cc +++ b/python/google/protobuf/pyext/repeated_scalar_container.cc @@ -105,7 +105,7 @@ static int AssignItem(RepeatedScalarContainer* self, if (arg == NULL) { ScopedPyObjectPtr py_index(PyLong_FromLong(index)); return cmessage::InternalDeleteRepeatedField(self->parent, field_descriptor, - py_index, NULL); + py_index.get(), NULL); } if (PySequence_Check(arg) && !(PyBytes_Check(arg) || PyUnicode_Check(arg))) { @@ -172,7 +172,7 @@ static int AssignItem(RepeatedScalarContainer* self, ScopedPyObjectPtr s(PyObject_Str(arg)); if (s != NULL) { PyErr_Format(PyExc_ValueError, "Unknown enum value: %s", - PyString_AsString(s)); + PyString_AsString(s.get())); } return -1; } @@ -334,7 +334,7 @@ static PyObject* Subscript(RepeatedScalarContainer* self, PyObject* slice) { break; } ScopedPyObjectPtr s(Item(self, index)); - PyList_Append(list, s); + PyList_Append(list, s.get()); } } else { if (step > 0) { @@ -345,7 +345,7 @@ static PyObject* Subscript(RepeatedScalarContainer* self, PyObject* slice) { break; } ScopedPyObjectPtr s(Item(self, index)); - PyList_Append(list, s); + PyList_Append(list, s.get()); } } return list; @@ -414,7 +414,7 @@ PyObject* Append(RepeatedScalarContainer* self, PyObject* item) { ScopedPyObjectPtr s(PyObject_Str(item)); if (s != NULL) { PyErr_Format(PyExc_ValueError, "Unknown enum value: %s", - PyString_AsString(s)); + PyString_AsString(s.get())); } return NULL; } @@ -483,15 +483,15 @@ static int AssSubscript(RepeatedScalarContainer* self, if (full_slice == NULL) { return -1; } - ScopedPyObjectPtr new_list(Subscript(self, full_slice)); + ScopedPyObjectPtr new_list(Subscript(self, full_slice.get())); if (new_list == NULL) { return -1; } - if (PySequence_SetSlice(new_list, from, to, value) < 0) { + if (PySequence_SetSlice(new_list.get(), from, to, value) < 0) { return -1; } - return InternalAssignRepeatedField(self, new_list); + return InternalAssignRepeatedField(self, new_list.get()); } PyObject* Extend(RepeatedScalarContainer* self, PyObject* value) { @@ -511,8 +511,8 @@ PyObject* Extend(RepeatedScalarContainer* self, PyObject* value) { return NULL; } ScopedPyObjectPtr next; - while ((next.reset(PyIter_Next(iter))) != NULL) { - if (ScopedPyObjectPtr(Append(self, next)) == NULL) { + while ((next.reset(PyIter_Next(iter.get()))) != NULL) { + if (ScopedPyObjectPtr(Append(self, next.get())) == NULL) { return NULL; } } @@ -529,11 +529,11 @@ static PyObject* Insert(RepeatedScalarContainer* self, PyObject* args) { return NULL; } ScopedPyObjectPtr full_slice(PySlice_New(NULL, NULL, NULL)); - ScopedPyObjectPtr new_list(Subscript(self, full_slice)); - if (PyList_Insert(new_list, index, value) < 0) { + ScopedPyObjectPtr new_list(Subscript(self, full_slice.get())); + if (PyList_Insert(new_list.get(), index, value) < 0) { return NULL; } - int ret = InternalAssignRepeatedField(self, new_list); + int ret = InternalAssignRepeatedField(self, new_list.get()); if (ret < 0) { return NULL; } @@ -544,7 +544,7 @@ static PyObject* Remove(RepeatedScalarContainer* self, PyObject* value) { Py_ssize_t match_index = -1; for (Py_ssize_t i = 0; i < Len(self); ++i) { ScopedPyObjectPtr elem(Item(self, i)); - if (PyObject_RichCompareBool(elem, value, Py_EQ)) { + if (PyObject_RichCompareBool(elem.get(), value, Py_EQ)) { match_index = i; break; } @@ -579,15 +579,15 @@ static PyObject* RichCompare(RepeatedScalarContainer* self, ScopedPyObjectPtr other_list_deleter; if (PyObject_TypeCheck(other, &RepeatedScalarContainer_Type)) { other_list_deleter.reset(Subscript( - reinterpret_cast(other), full_slice)); + reinterpret_cast(other), full_slice.get())); other = other_list_deleter.get(); } - ScopedPyObjectPtr list(Subscript(self, full_slice)); + ScopedPyObjectPtr list(Subscript(self, full_slice.get())); if (list == NULL) { return NULL; } - return PyObject_RichCompare(list, other, opid); + return PyObject_RichCompare(list.get(), other, opid); } PyObject* Reduce(RepeatedScalarContainer* unused_self) { @@ -618,19 +618,19 @@ static PyObject* Sort(RepeatedScalarContainer* self, if (full_slice == NULL) { return NULL; } - ScopedPyObjectPtr list(Subscript(self, full_slice)); + ScopedPyObjectPtr list(Subscript(self, full_slice.get())); if (list == NULL) { return NULL; } - ScopedPyObjectPtr m(PyObject_GetAttrString(list, "sort")); + ScopedPyObjectPtr m(PyObject_GetAttrString(list.get(), "sort")); if (m == NULL) { return NULL; } - ScopedPyObjectPtr res(PyObject_Call(m, args, kwds)); + ScopedPyObjectPtr res(PyObject_Call(m.get(), args, kwds)); if (res == NULL) { return NULL; } - int ret = InternalAssignRepeatedField(self, list); + int ret = InternalAssignRepeatedField(self, list.get()); if (ret < 0) { return NULL; } @@ -688,7 +688,7 @@ static int InitializeAndCopyToParentContainer( if (full_slice == NULL) { return -1; } - ScopedPyObjectPtr values(Subscript(from, full_slice)); + ScopedPyObjectPtr values(Subscript(from, full_slice.get())); if (values == NULL) { return -1; } @@ -697,7 +697,7 @@ static int InitializeAndCopyToParentContainer( to->parent_field_descriptor = from->parent_field_descriptor; to->message = new_message; to->owner.reset(new_message); - if (InternalAssignRepeatedField(to, values) < 0) { + if (InternalAssignRepeatedField(to, values.get()) < 0) { return -1; } return 0; diff --git a/python/google/protobuf/pyext/scalar_map_container.cc b/python/google/protobuf/pyext/scalar_map_container.cc index a355edb2..0b0d5a3d 100644 --- a/python/google/protobuf/pyext/scalar_map_container.cc +++ b/python/google/protobuf/pyext/scalar_map_container.cc @@ -94,7 +94,7 @@ PyObject *NewContainer( "Could not allocate new container."); } - ScalarMapContainer* self = GetMap(obj); + ScalarMapContainer* self = GetMap(obj.get()); self->message = parent->message; self->parent = parent; @@ -160,7 +160,7 @@ int MapKeyMatches(ScalarMapContainer* self, const Message* entry, // TODO(haberman): do we need more strict type checking? ScopedPyObjectPtr entry_key( cmessage::InternalGetScalar(entry, self->key_field_descriptor)); - int ret = PyObject_RichCompareBool(key, entry_key, Py_EQ); + int ret = PyObject_RichCompareBool(key, entry_key.get(), Py_EQ); return ret; } @@ -251,7 +251,7 @@ int SetItem(PyObject *_self, PyObject *key, PyObject *v) { if (matches < 0) return -1; if (matches) { found = true; - if (i != size - 1) { + if (i != (int)size - 1) { reflection->SwapElements(message, self->parent_field_descriptor, i, size - 1); } @@ -303,7 +303,7 @@ PyObject* GetIterator(PyObject *_self) { // TODO(haberman): add lookup API to Reflection API. size_t size = reflection->FieldSize(*message, self->parent_field_descriptor); - for (int i = 0; i < size; i++) { + for (size_t i = 0; i < size; i++) { Message* entry = reflection->MutableRepeatedMessage( message, self->parent_field_descriptor, i); ScopedPyObjectPtr key( @@ -382,12 +382,6 @@ PyObject* Get(PyObject* self, PyObject* args) { } } -static PyMappingMethods MpMethods = { - Length, // mp_length - GetItem, // mp_subscript - SetItem, // mp_ass_subscript -}; - static void Dealloc(PyObject* _self) { ScalarMapContainer* self = GetMap(_self); self->owner.reset(); @@ -458,6 +452,12 @@ PyObject* IterNext(PyObject* _self) { }; PyObject *ScalarMapContainer_Type; #else + static PyMappingMethods MpMethods = { + scalar_map_container::Length, // mp_length + scalar_map_container::GetItem, // mp_subscript + scalar_map_container::SetItem, // mp_ass_subscript + }; + PyTypeObject ScalarMapContainer_Type = { PyVarObject_HEAD_INIT(&PyType_Type, 0) FULL_MODULE_NAME ".ScalarMapContainer", // tp_name @@ -471,7 +471,7 @@ PyObject* IterNext(PyObject* _self) { 0, // tp_repr 0, // tp_as_number 0, // tp_as_sequence - &scalar_map_container::MpMethods, // tp_as_mapping + &MpMethods, // tp_as_mapping 0, // tp_hash 0, // tp_call 0, // tp_str diff --git a/python/google/protobuf/pyext/scoped_pyobject_ptr.h b/python/google/protobuf/pyext/scoped_pyobject_ptr.h index 9979b83b..a128cd4c 100644 --- a/python/google/protobuf/pyext/scoped_pyobject_ptr.h +++ b/python/google/protobuf/pyext/scoped_pyobject_ptr.h @@ -60,11 +60,6 @@ class ScopedPyObjectPtr { return ptr_; } - // ScopedPyObjectPtr should not be copied. - // We explicitly list and delete this overload to avoid automatic conversion - // to PyObject*, which is wrong in this case. - PyObject* reset(const ScopedPyObjectPtr& other) = delete; - // Releases ownership of the object. // The caller now owns the returned reference. PyObject* release() { @@ -73,8 +68,6 @@ class ScopedPyObjectPtr { return p; } - operator PyObject*() { return ptr_; } - PyObject* operator->() const { assert(ptr_ != NULL); return ptr_; diff --git a/python/setup.py b/python/setup.py index cda56867..662a145a 100755 --- a/python/setup.py +++ b/python/setup.py @@ -148,17 +148,24 @@ class build_py(_build_py): if __name__ == '__main__': ext_module_list = [] cpp_impl = '--cpp_implementation' + warnings_as_errors = '--warnings_as_errors' if cpp_impl in sys.argv: sys.argv.remove(cpp_impl) + extra_compile_args = ['-Wno-write-strings'] + + if warnings_as_errors in sys.argv: + extra_compile_args.append('-Werror') + sys.argv.remove(warnings_as_errors) + # C++ implementation extension ext_module_list.append( Extension( "google.protobuf.pyext._message", glob.glob('google/protobuf/pyext/*.cc'), - define_macros=[('GOOGLE_PROTOBUF_HAS_ONEOF', '1')], include_dirs=[".", "../src"], libraries=['protobuf'], library_dirs=['../src/.libs'], + extra_compile_args=extra_compile_args, ) ) os.environ['PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION'] = 'cpp' diff --git a/python/tox.ini b/python/tox.ini index a6352ef4..7deb3ba3 100644 --- a/python/tox.ini +++ b/python/tox.ini @@ -13,7 +13,7 @@ setenv = commands = python setup.py -q build_py python: python setup.py -q build - cpp: python setup.py -q build --cpp_implementation + cpp: python setup.py -q build --cpp_implementation --warnings_as_errors python: python setup.py -q test -q cpp: python setup.py -q test -q --cpp_implementation deps = -- cgit v1.2.3