From 16adea3ddc4e775aac230c4326aeb686c26620dc Mon Sep 17 00:00:00 2001 From: Feng Xiao Date: Fri, 22 Jul 2016 14:15:19 -0700 Subject: Add a test to catch sign-comparison warnings. grpc build treates them as errors and such issues (protobuf change breaks grpc) has been reported repeatedly. For example: https://github.com/google/protobuf/issues/1813 Change-Id: I077c4557cf3effd5195f88802c38999b884edc30 --- src/Makefile.am | 34 ++- src/google/protobuf/repeated_field_reflection.h | 337 ------------------------ 2 files changed, 28 insertions(+), 343 deletions(-) delete mode 100644 src/google/protobuf/repeated_field_reflection.h (limited to 'src') diff --git a/src/Makefile.am b/src/Makefile.am index 996fafa8..6e7dacfc 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -19,8 +19,9 @@ PTHREAD_DEF = endif if GCC -# These are good warnings to turn on by default -NO_OPT_CXXFLAGS = $(PTHREAD_CFLAGS) $(PTHREAD_DEF) $(ZLIB_DEF) -Wall -Wwrite-strings -Woverloaded-virtual -Wno-sign-compare +# Turn on all warnings except for sign comparison (we ignore sign comparison +# in Google so our code base have tons of such warnings). +NO_OPT_CXXFLAGS = $(PTHREAD_CFLAGS) $(PTHREAD_DEF) $(ZLIB_DEF) -Wall -Wno-sign-compare else NO_OPT_CXXFLAGS = $(PTHREAD_CFLAGS) $(PTHREAD_DEF) $(ZLIB_DEF) endif @@ -50,7 +51,8 @@ clean-local: rm -f *.loT CLEANFILES = $(protoc_outputs) unittest_proto_middleman \ - testzip.jar testzip.list testzip.proto testzip.zip + testzip.jar testzip.list testzip.proto testzip.zip \ + no_warning_test.cc MAINTAINERCLEANFILES = \ Makefile.in @@ -122,7 +124,6 @@ nobase_include_HEADERS = \ google/protobuf/reflection.h \ google/protobuf/reflection_ops.h \ google/protobuf/repeated_field.h \ - google/protobuf/repeated_field_reflection.h \ google/protobuf/service.h \ google/protobuf/source_context.pb.h \ google/protobuf/struct.pb.h \ @@ -680,7 +681,7 @@ COMMON_TEST_SOURCES = \ check_PROGRAMS = protoc protobuf-test protobuf-lazy-descriptor-test \ protobuf-lite-test test_plugin protobuf-lite-arena-test \ - $(GZCHECKPROGRAMS) + no-warning-test $(GZCHECKPROGRAMS) protobuf_test_LDADD = $(PTHREAD_LIBS) libprotobuf.la libprotoc.la \ ../gmock/gtest/lib/libgtest.la \ ../gmock/lib/libgmock.la \ @@ -833,6 +834,27 @@ zcgunzip_LDADD = $(PTHREAD_LIBS) libprotobuf.la zcgunzip_SOURCES = google/protobuf/testing/zcgunzip.cc endif +# This test target is to ensure all our public header files and generated +# code is free from warnings. We have to be more pedantic about these +# files because they are compiled by users with different compiler flags. +no_warning_test.cc: + echo "// Generated from Makefile.am" > no_warning_test.cc + for FILE in $(nobase_include_HEADERS); do \ + if ! echo $${FILE} | grep "atomicops"; then \ + echo "#include <$${FILE}>" >> no_warning_test.cc; \ + fi \ + done + echo "#include " >> no_warning_test.cc + echo "TEST(NoWarningTest, Empty) {}" >> no_warning_test.cc + +no_warning_test_LDADD = $(PTHREAD_LIBS) libprotobuf.la \ + ../gmock/gtest/lib/libgtest.la \ + ../gmock/gtest/lib/libgtest_main.la +no_warning_test_CPPFLAGS = -I$(srcdir)/../gmock/gtest/include +no_warning_test_CXXFLAGS = $(PTHREAD_CFLAGS) $(PTHREAD_DEF) $(ZLIB_DEF) \ + -Wall -Werror +nodist_no_warning_test_SOURCES = no_warning_test.cc $(protoc_outputs) + TESTS = protobuf-test protobuf-lazy-descriptor-test protobuf-lite-test \ google/protobuf/compiler/zip_output_unittest.sh $(GZTESTS) \ - protobuf-lite-arena-test + protobuf-lite-arena-test no-warning-test diff --git a/src/google/protobuf/repeated_field_reflection.h b/src/google/protobuf/repeated_field_reflection.h deleted file mode 100644 index 512c0f1d..00000000 --- a/src/google/protobuf/repeated_field_reflection.h +++ /dev/null @@ -1,337 +0,0 @@ -// Protocol Buffers - Google's data interchange format -// Copyright 2008 Google Inc. All rights reserved. -// https://developers.google.com/protocol-buffers/ -// -// Redistribution and use in source and binary forms, with or without -// modification, are permitted provided that the following conditions are -// met: -// -// * Redistributions of source code must retain the above copyright -// notice, this list of conditions and the following disclaimer. -// * Redistributions in binary form must reproduce the above -// copyright notice, this list of conditions and the following disclaimer -// in the documentation and/or other materials provided with the -// distribution. -// * Neither the name of Google Inc. nor the names of its -// contributors may be used to endorse or promote products derived from -// this software without specific prior written permission. -// -// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS -// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT -// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR -// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT -// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, -// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT -// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, -// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY -// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT -// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE -// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - -// This header file is protobuf internal. Users should not include this -// file directly. -#ifndef GOOGLE_PROTOBUF_REPEATED_FIELD_REFLECTION_H__ -#define GOOGLE_PROTOBUF_REPEATED_FIELD_REFLECTION_H__ - -#include -#ifndef _SHARED_PTR_H -#include -#endif - -#include - -namespace google { -namespace protobuf { -namespace internal { -// Interfaces used to implement reflection RepeatedFieldRef API. -// Reflection::GetRepeatedAccessor() should return a pointer to an singleton -// object that implements the below interface. -// -// This interface passes/returns values using void pointers. The actual type -// of the value depends on the field's cpp_type. Following is a mapping from -// cpp_type to the type that should be used in this interface: -// -// field->cpp_type() T Actual type of void* -// CPPTYPE_INT32 int32 int32 -// CPPTYPE_UINT32 uint32 uint32 -// CPPTYPE_INT64 int64 int64 -// CPPTYPE_UINT64 uint64 uint64 -// CPPTYPE_DOUBLE double double -// CPPTYPE_FLOAT float float -// CPPTYPE_BOOL bool bool -// CPPTYPE_ENUM generated enum type int32 -// CPPTYPE_STRING string string -// CPPTYPE_MESSAGE generated message type google::protobuf::Message -// or google::protobuf::Message -// -// Note that for enums we use int32 in the interface. -// -// You can map from T to the actual type using RefTypeTraits: -// typedef RefTypeTraits::AccessorValueType ActualType; -class LIBPROTOBUF_EXPORT RepeatedFieldAccessor { - public: - // Typedefs for clarity. - typedef void Field; - typedef void Value; - typedef void Iterator; - - virtual ~RepeatedFieldAccessor(); - virtual bool IsEmpty(const Field* data) const = 0; - virtual int Size(const Field* data) const = 0; - // Depends on the underlying representation of the repeated field, this - // method can return a pointer to the underlying object if such an object - // exists, or fill the data into scratch_space and return scratch_space. - // Callers of this method must ensure scratch_space is a valid pointer - // to a mutable object of the correct type. - virtual const Value* Get( - const Field* data, int index, Value* scratch_space) const = 0; - - virtual void Clear(Field* data) const = 0; - virtual void Set(Field* data, int index, const Value* value) const = 0; - virtual void Add(Field* data, const Value* value) const = 0; - virtual void RemoveLast(Field* data) const = 0; - virtual void SwapElements(Field* data, int index1, int index2) const = 0; - virtual void Swap(Field* data, const RepeatedFieldAccessor* other_mutator, - Field* other_data) const = 0; - - // Create an iterator that points at the beginning of the repeated field. - virtual Iterator* BeginIterator(const Field* data) const = 0; - // Create an iterator that points at the end of the repeated field. - virtual Iterator* EndIterator(const Field* data) const = 0; - // Make a copy of an iterator and return the new copy. - virtual Iterator* CopyIterator(const Field* data, - const Iterator* iterator) const = 0; - // Move an iterator to point to the next element. - virtual Iterator* AdvanceIterator(const Field* data, - Iterator* iterator) const = 0; - // Compare whether two iterators point to the same element. - virtual bool EqualsIterator(const Field* data, const Iterator* a, - const Iterator* b) const = 0; - // Delete an iterator created by BeginIterator(), EndIterator() and - // CopyIterator(). - virtual void DeleteIterator(const Field* data, Iterator* iterator) const = 0; - // Like Get() but for iterators. - virtual const Value* GetIteratorValue(const Field* data, - const Iterator* iterator, - Value* scratch_space) const = 0; - - // Templated methods that make using this interface easier for non-message - // types. - template - T Get(const Field* data, int index) const { - typedef typename RefTypeTraits::AccessorValueType ActualType; - ActualType scratch_space; - return static_cast( - *reinterpret_cast( - Get(data, index, static_cast(&scratch_space)))); - } - - template - void Set(Field* data, int index, const ValueType& value) const { - typedef typename RefTypeTraits::AccessorValueType ActualType; - // In this RepeatedFieldAccessor interface we pass/return data using - // raw pointers. Type of the data these raw pointers point to should - // be ActualType. Here we have a ValueType object and want a ActualType - // pointer. We can't cast a ValueType pointer to an ActualType pointer - // directly because their type might be different (for enums ValueType - // may be a generated enum type while ActualType is int32). To be safe - // we make a copy to get a temporary ActualType object and use it. - ActualType tmp = static_cast(value); - Set(data, index, static_cast(&tmp)); - } - - template - void Add(Field* data, const ValueType& value) const { - typedef typename RefTypeTraits::AccessorValueType ActualType; - // In this RepeatedFieldAccessor interface we pass/return data using - // raw pointers. Type of the data these raw pointers point to should - // be ActualType. Here we have a ValueType object and want a ActualType - // pointer. We can't cast a ValueType pointer to an ActualType pointer - // directly because their type might be different (for enums ValueType - // may be a generated enum type while ActualType is int32). To be safe - // we make a copy to get a temporary ActualType object and use it. - ActualType tmp = static_cast(value); - Add(data, static_cast(&tmp)); - } -}; - -// Implement (Mutable)RepeatedFieldRef::iterator -template -class RepeatedFieldRefIterator - : public std::iterator { - typedef typename RefTypeTraits::AccessorValueType AccessorValueType; - typedef typename RefTypeTraits::IteratorValueType IteratorValueType; - typedef typename RefTypeTraits::IteratorPointerType IteratorPointerType; - - public: - // Constructor for non-message fields. - RepeatedFieldRefIterator(const void* data, - const RepeatedFieldAccessor* accessor, - bool begin) - : data_(data), accessor_(accessor), - iterator_(begin ? accessor->BeginIterator(data) : - accessor->EndIterator(data)), - scratch_space_(new AccessorValueType) { - } - // Constructor for message fields. - RepeatedFieldRefIterator(const void* data, - const RepeatedFieldAccessor* accessor, - bool begin, - AccessorValueType* scratch_space) - : data_(data), accessor_(accessor), - iterator_(begin ? accessor->BeginIterator(data) : - accessor->EndIterator(data)), - scratch_space_(scratch_space) { - } - ~RepeatedFieldRefIterator() { - accessor_->DeleteIterator(data_, iterator_); - } - RepeatedFieldRefIterator operator++(int) { - RepeatedFieldRefIterator tmp(*this); - iterator_ = accessor_->AdvanceIterator(data_, iterator_); - return tmp; - } - RepeatedFieldRefIterator& operator++() { - iterator_ = accessor_->AdvanceIterator(data_, iterator_); - return *this; - } - IteratorValueType operator*() const { - return static_cast( - *static_cast( - accessor_->GetIteratorValue( - data_, iterator_, scratch_space_.get()))); - } - IteratorPointerType operator->() const { - return static_cast( - accessor_->GetIteratorValue( - data_, iterator_, scratch_space_.get())); - } - bool operator!=(const RepeatedFieldRefIterator& other) const { - assert(data_ == other.data_); - assert(accessor_ == other.accessor_); - return !accessor_->EqualsIterator(data_, iterator_, other.iterator_); - } - bool operator==(const RepeatedFieldRefIterator& other) const { - return !this->operator!=(other); - } - - RepeatedFieldRefIterator(const RepeatedFieldRefIterator& other) - : data_(other.data_), accessor_(other.accessor_), - iterator_(accessor_->CopyIterator(data_, other.iterator_)) { - } - RepeatedFieldRefIterator& operator=(const RepeatedFieldRefIterator& other) { - if (this != &other) { - accessor_->DeleteIterator(data_, iterator_); - data_ = other.data_; - accessor_ = other.accessor_; - iterator_ = accessor_->CopyIterator(data_, other.iterator_); - } - return *this; - } - - protected: - const void* data_; - const RepeatedFieldAccessor* accessor_; - void* iterator_; - google::protobuf::scoped_ptr scratch_space_; -}; - -// TypeTraits that maps the type parameter T of RepeatedFieldRef or -// MutableRepeatedFieldRef to corresponding iterator type, -// RepeatedFieldAccessor type, etc. -template -struct PrimitiveTraits { - static const bool is_primitive = false; -}; -#define DEFINE_PRIMITIVE(TYPE, type) \ - template<> struct PrimitiveTraits { \ - static const bool is_primitive = true; \ - static const FieldDescriptor::CppType cpp_type = \ - FieldDescriptor::CPPTYPE_ ## TYPE; \ - }; -DEFINE_PRIMITIVE(INT32, int32) -DEFINE_PRIMITIVE(UINT32, uint32) -DEFINE_PRIMITIVE(INT64, int64) -DEFINE_PRIMITIVE(UINT64, uint64) -DEFINE_PRIMITIVE(FLOAT, float) -DEFINE_PRIMITIVE(DOUBLE, double) -DEFINE_PRIMITIVE(BOOL, bool) -#undef DEFINE_PRIMITIVE - -template -struct RefTypeTraits< - T, typename internal::enable_if::is_primitive>::type> { - typedef RepeatedFieldRefIterator iterator; - typedef RepeatedFieldAccessor AccessorType; - typedef T AccessorValueType; - typedef T IteratorValueType; - typedef T* IteratorPointerType; - static const FieldDescriptor::CppType cpp_type = - PrimitiveTraits::cpp_type; - static const Descriptor* GetMessageFieldDescriptor() { - return NULL; - } -}; - -template -struct RefTypeTraits< - T, typename internal::enable_if::value>::type> { - typedef RepeatedFieldRefIterator iterator; - typedef RepeatedFieldAccessor AccessorType; - // We use int32 for repeated enums in RepeatedFieldAccessor. - typedef int32 AccessorValueType; - typedef T IteratorValueType; - typedef int32* IteratorPointerType; - static const FieldDescriptor::CppType cpp_type = - FieldDescriptor::CPPTYPE_ENUM; - static const Descriptor* GetMessageFieldDescriptor() { - return NULL; - } -}; - -template -struct RefTypeTraits< - T, typename internal::enable_if::value>::type> { - typedef RepeatedFieldRefIterator iterator; - typedef RepeatedFieldAccessor AccessorType; - typedef string AccessorValueType; - typedef string IteratorValueType; - typedef string* IteratorPointerType; - static const FieldDescriptor::CppType cpp_type = - FieldDescriptor::CPPTYPE_STRING; - static const Descriptor* GetMessageFieldDescriptor() { - return NULL; - } -}; - -template -struct MessageDescriptorGetter { - static const Descriptor* get() { - return T::default_instance().GetDescriptor(); - } -}; -template<> -struct MessageDescriptorGetter { - static const Descriptor* get() { - return NULL; - } -}; - -template -struct RefTypeTraits< - T, typename internal::enable_if::value>::type> { - typedef RepeatedFieldRefIterator iterator; - typedef RepeatedFieldAccessor AccessorType; - typedef Message AccessorValueType; - typedef const T& IteratorValueType; - typedef const T* IteratorPointerType; - static const FieldDescriptor::CppType cpp_type = - FieldDescriptor::CPPTYPE_MESSAGE; - static const Descriptor* GetMessageFieldDescriptor() { - return MessageDescriptorGetter::get(); - } -}; -} // namespace internal -} // namespace protobuf -} // namespace google -#endif // GOOGLE_PROTOBUF_REPEATED_FIELD_REFLECTION_H__ -- cgit v1.2.3 From 9009662b719f98dee6c352d05a7630612313c58e Mon Sep 17 00:00:00 2001 From: Feng Xiao Date: Fri, 22 Jul 2016 21:41:17 +0000 Subject: Fix sign-comparison warnings in public header files. --- src/google/protobuf/map.h | 2 +- src/google/protobuf/wire_format_lite_inl.h | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) (limited to 'src') diff --git a/src/google/protobuf/map.h b/src/google/protobuf/map.h index 42bcfd94..1b9aa703 100644 --- a/src/google/protobuf/map.h +++ b/src/google/protobuf/map.h @@ -1250,7 +1250,7 @@ class Map { // Return whether table_[b] is a linked list that seems awfully long. // Requires table_[b] to point to a non-empty linked list. bool TableEntryIsTooLong(size_type b) { - const int kMaxLength = 8; + const size_type kMaxLength = 8; size_type count = 0; Node* node = static_cast(table_[b]); do { diff --git a/src/google/protobuf/wire_format_lite_inl.h b/src/google/protobuf/wire_format_lite_inl.h index ebd858ff..93d7c824 100644 --- a/src/google/protobuf/wire_format_lite_inl.h +++ b/src/google/protobuf/wire_format_lite_inl.h @@ -346,9 +346,9 @@ inline bool WireFormatLite::ReadPackedFixedSizePrimitive( io::CodedInputStream* input, RepeatedField* values) { int length; if (!input->ReadVarintSizeAsInt(&length)) return false; - const uint32 old_entries = values->size(); - const uint32 new_entries = length / sizeof(CType); - const uint32 new_bytes = new_entries * sizeof(CType); + const int old_entries = values->size(); + const int new_entries = length / sizeof(CType); + const int new_bytes = new_entries * sizeof(CType); if (new_bytes != length) return false; // We would *like* to pre-allocate the buffer to write into (for // speed), but *must* avoid performing a very large allocation due @@ -382,7 +382,7 @@ inline bool WireFormatLite::ReadPackedFixedSizePrimitive( #else values->Reserve(old_entries + new_entries); CType value; - for (uint32 i = 0; i < new_entries; ++i) { + for (int i = 0; i < new_entries; ++i) { if (!ReadPrimitive(input, &value)) return false; values->AddAlreadyReserved(value); } @@ -392,7 +392,7 @@ inline bool WireFormatLite::ReadPackedFixedSizePrimitive( // safely allocate. We read as much as we can into *values // without pre-allocating "length" bytes. CType value; - for (uint32 i = 0; i < new_entries; ++i) { + for (int i = 0; i < new_entries; ++i) { if (!ReadPrimitive(input, &value)) return false; values->Add(value); } -- cgit v1.2.3