From beb00de28d90cab43edad22ca098b7993080485c Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Wed, 14 Feb 2018 11:30:20 -0500 Subject: [PATCH 1/4] Enable -Wcomma for our build; disable it for abseil. In order to use different cflags for abseil, this patch splits it out into a subspec within the pod. The cmake side of things "just works" since Firestore/CMakeLists.txt includes abseil before setting our compiler flags. --- FirebaseFirestore.podspec | 18 ++++++++++++++++-- .../firebase/firestore/remote/serializer.cc | 12 ++++++++++++ cmake/CompilerSetup.cmake | 3 +++ 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/FirebaseFirestore.podspec b/FirebaseFirestore.podspec index 0f0fe1973b8..7a5ad111f04 100644 --- a/FirebaseFirestore.podspec +++ b/FirebaseFirestore.podspec @@ -35,7 +35,6 @@ Google Cloud Firestore is a NoSQL document database built for automatic scaling, 'Firestore/core/include/**/*.{h,cc,mm}', 'Firestore/core/src/**/*.{h,cc,mm}', 'Firestore/third_party/Immutable/*.[mh]', - 'Firestore/third_party/abseil-cpp/**/*.{h,cc}' ] s.requires_arc = [ 'Firestore/Source/**/*', @@ -45,7 +44,6 @@ Google Cloud Firestore is a NoSQL document database built for automatic scaling, s.exclude_files = [ 'Firestore/Port/*test.cc', 'Firestore/third_party/Immutable/Tests/**', - 'Firestore/third_party/abseil-cpp/**/*_test.{h,cc}', # Exclude alternate implementations for other platforms 'Firestore/core/src/firebase/firestore/util/assert_stdio.cc', @@ -79,4 +77,20 @@ Google Cloud Firestore is a NoSQL document database built for automatic scaling, Firestore/core/src/firebase/firestore/util/config.h.in > \ Firestore/core/src/firebase/firestore/util/config.h CMD + + s.subspec 'abseil-cpp' do |ss| + ss.source_files = [ + 'Firestore/third_party/abseil-cpp/**/*.{h,cc}' + ] + ss.exclude_files = [ + 'Firestore/third_party/abseil-cpp/**/*_test.{h,cc}', + ] + + # NB: don't include absl/**/*.h, as that would include absl/*/internal/*.h. + ss.public_header_files = 'Firestore/third_party/abseil-cpp/absl/*/*.h' + ss.header_mappings_dir = 'Firestore/third_party/abseil-cpp' + + ss.library = 'c++' + ss.compiler_flags = '$(inherited) ' + '-Wno-comma' + end end diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc index e503d0926ee..4d8ca0ad66e 100644 --- a/Firestore/core/src/firebase/firestore/remote/serializer.cc +++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc @@ -25,6 +25,18 @@ namespace remote { using firebase::firestore::model::FieldValue; +// TODO: DO NOT SUBMIT. This code exists solely to generate an error (via the +// -Wcomma flag) and should be removed before we submit. Uncomment this to see +// the error. (Treated as a non-fatal warning by xcode, but treated as fatal by +// cmake due to -Werror. +/* +void GenerateCompilerError() { + int x = 0; + int y = 0; + x++, y++; // Compiler warning due to -Wcomma here. +} +*/ + Serializer::TypedValue Serializer::EncodeFieldValue( const FieldValue& field_value) { Serializer::TypedValue proto_value{ diff --git a/cmake/CompilerSetup.cmake b/cmake/CompilerSetup.cmake index aa4289ac541..8e41209245d 100644 --- a/cmake/CompilerSetup.cmake +++ b/cmake/CompilerSetup.cmake @@ -32,6 +32,9 @@ if(CLANG OR GNU) common_flags -Wall -Wextra -Werror + # Detect possible misuse of comma operator. + -Wcomma + # Be super pedantic about format strings -Wformat From 496d0dfcb6a5bda4b7f138177f9b387d970e66f3 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Wed, 14 Feb 2018 16:39:28 -0500 Subject: [PATCH 2/4] Remove example that triggers -Wcomma --- .../core/src/firebase/firestore/remote/serializer.cc | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc index 4d8ca0ad66e..e503d0926ee 100644 --- a/Firestore/core/src/firebase/firestore/remote/serializer.cc +++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc @@ -25,18 +25,6 @@ namespace remote { using firebase::firestore::model::FieldValue; -// TODO: DO NOT SUBMIT. This code exists solely to generate an error (via the -// -Wcomma flag) and should be removed before we submit. Uncomment this to see -// the error. (Treated as a non-fatal warning by xcode, but treated as fatal by -// cmake due to -Werror. -/* -void GenerateCompilerError() { - int x = 0; - int y = 0; - x++, y++; // Compiler warning due to -Wcomma here. -} -*/ - Serializer::TypedValue Serializer::EncodeFieldValue( const FieldValue& field_value) { Serializer::TypedValue proto_value{ From af975f6ec2f9068f71839410d8786f358da15b60 Mon Sep 17 00:00:00 2001 From: Gil Date: Thu, 15 Feb 2018 06:14:31 -0800 Subject: [PATCH 3/4] Don't expose abseil headers as public headers (#803) --- FirebaseFirestore.podspec | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/FirebaseFirestore.podspec b/FirebaseFirestore.podspec index 7a5ad111f04..13a0a85b2ba 100644 --- a/FirebaseFirestore.podspec +++ b/FirebaseFirestore.podspec @@ -79,17 +79,16 @@ Google Cloud Firestore is a NoSQL document database built for automatic scaling, CMD s.subspec 'abseil-cpp' do |ss| + ss.preserve_path = [ + 'Firestore/third_party/abseil-cpp/absl' + ] ss.source_files = [ - 'Firestore/third_party/abseil-cpp/**/*.{h,cc}' + 'Firestore/third_party/abseil-cpp/**/*.cc' ] ss.exclude_files = [ - 'Firestore/third_party/abseil-cpp/**/*_test.{h,cc}', + 'Firestore/third_party/abseil-cpp/**/*_test.cc', ] - # NB: don't include absl/**/*.h, as that would include absl/*/internal/*.h. - ss.public_header_files = 'Firestore/third_party/abseil-cpp/absl/*/*.h' - ss.header_mappings_dir = 'Firestore/third_party/abseil-cpp' - ss.library = 'c++' ss.compiler_flags = '$(inherited) ' + '-Wno-comma' end From a239369995a2c8b43b19b72b7e069d536cdccacb Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Thu, 15 Feb 2018 09:36:12 -0500 Subject: [PATCH 4/4] Move -Wcomma option to apply to clang only It's not a supported option under g++ --- cmake/CompilerSetup.cmake | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cmake/CompilerSetup.cmake b/cmake/CompilerSetup.cmake index 8e41209245d..b5600279d8e 100644 --- a/cmake/CompilerSetup.cmake +++ b/cmake/CompilerSetup.cmake @@ -32,9 +32,6 @@ if(CLANG OR GNU) common_flags -Wall -Wextra -Werror - # Detect possible misuse of comma operator. - -Wcomma - # Be super pedantic about format strings -Wformat @@ -64,6 +61,9 @@ if(CLANG OR GNU) APPEND common_flags -Wconditional-uninitialized -Werror=return-type -Winfinite-recursion -Wmove -Wrange-loop-analysis -Wunreachable-code + + # Options added to match apple recommended project settings + -Wcomma ) endif()