Merge pull request #861 from haberman/pywarnings

Removed all warnings from the Python/C++ build

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;

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;
     }

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[] = {

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 {

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

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<Message*>(&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<CMessage*>(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<RepeatedCompositeContainer*>(other), full_slice));
+        Subscript(reinterpret_cast<RepeatedCompositeContainer*>(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;

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.
 //

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<RepeatedScalarContainer*>(other), full_slice));
+        reinterpret_cast<RepeatedScalarContainer*>(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;

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

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_;

python/setup.py

@@ -148,17 +148,27 @@ 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', '-Wno-invalid-offsetof']
+
+    if "clang" in os.popen('$CC --version').read():
+      extra_compile_args.append('-Wno-shorten-64-to-32')
+
+    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'

python/tox.ini

 [tox]
 envlist =
-    # cpp implementation on py34 is currently broken due to
-    # changes introduced by http://bugs.python.org/issue22079.
     py{26,27,33,34}-{cpp,python}
 
 [testenv]
 usedevelop=true
+passenv = CC
 setenv =
     cpp: LD_LIBRARY_PATH={toxinidir}/../src/.libs
     cpp: DYLD_LIBRARY_PATH={toxinidir}/../src/.libs
@@ -13,7 +12,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 =