[jira] [Commented] (PARQUET-1226) [C++] Fix new build warnings with clang 5.0

2018-02-20 Thread ASF GitHub Bot (JIRA)

[ 
https://issues.apache.org/jira/browse/PARQUET-1226?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16370318#comment-16370318
 ] 

ASF GitHub Bot commented on PARQUET-1226:
-

wesm closed pull request #442: PARQUET-1226: Fixes for CHECKIN compiler warning 
level with clang 5.0
URL: https://github.com/apache/parquet-cpp/pull/442
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/CMakeLists.txt b/CMakeLists.txt
index c2d4ef4f..bca8478c 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -84,7 +84,7 @@ enable_testing()
 set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${CMAKE_SOURCE_DIR}/cmake_modules")
 set(BUILD_SUPPORT_DIR "${CMAKE_SOURCE_DIR}/build-support")
 
-set(CLANG_FORMAT_VERSION "4.0")
+set(CLANG_FORMAT_VERSION "5.0")
 find_package(ClangTools)
 if ("$ENV{CMAKE_EXPORT_COMPILE_COMMANDS}" STREQUAL "1" OR CLANG_TIDY_FOUND)
   # Generate a Clang compile_commands.json "compilation database" file for use
@@ -160,6 +160,11 @@ if ("${CMAKE_SOURCE_DIR}" STREQUAL 
"${CMAKE_CURRENT_SOURCE_DIR}")
   "Build Parquet with statically linked CRT"
   OFF)
   endif()
+
+  option(PARQUET_VERBOSE_THIRDPARTY_BUILD
+"If off, output from ExternalProjects will be logged to files rather than 
shown"
+OFF)
+
 endif()
 
 include(BuildUtils)
diff --git a/cmake_modules/SetupCxxFlags.cmake 
b/cmake_modules/SetupCxxFlags.cmake
index 1678e8dc..01ed85bf 100644
--- a/cmake_modules/SetupCxxFlags.cmake
+++ b/cmake_modules/SetupCxxFlags.cmake
@@ -109,6 +109,11 @@ if ("${UPPERCASE_BUILD_WARNING_LEVEL}" STREQUAL "CHECKIN")
   set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-undefined-func-template")
 endif()
 
+if ("${COMPILER_VERSION}" VERSION_GREATER "4.0")
+  set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-unused-template \
+-Wno-zero-as-null-pointer-constant")
+endif()
+
 # Treat all compiler warnings as errors
 set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-unknown-warning-option 
-Werror")
   elseif ("${COMPILER_FAMILY}" STREQUAL "gcc")
diff --git a/cmake_modules/ThirdpartyToolchain.cmake 
b/cmake_modules/ThirdpartyToolchain.cmake
index 09e30dfe..9f241121 100644
--- a/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cmake_modules/ThirdpartyToolchain.cmake
@@ -30,6 +30,18 @@ if (NOT MSVC)
   set(EP_C_FLAGS "${EP_C_FLAGS} -fPIC")
 endif()
 
+if (NOT PARQUET_VERBOSE_THIRDPARTY_BUILD)
+  set(EP_LOG_OPTIONS
+LOG_CONFIGURE 1
+LOG_BUILD 1
+LOG_INSTALL 1
+LOG_DOWNLOAD 1)
+  set(Boost_DEBUG FALSE)
+else()
+  set(EP_LOG_OPTIONS)
+  set(Boost_DEBUG TRUE)
+endif()
+
 # --
 # Configure toolchain with environment variables, if the exist
 
@@ -52,7 +64,6 @@ endif()
 # Boost
 
 # find boost headers and libs
-set(Boost_DEBUG TRUE)
 set(Boost_USE_MULTITHREADED ON)
 if (MSVC AND PARQUET_USE_STATIC_CRT)
   set(Boost_USE_STATIC_RUNTIME ON)
@@ -168,6 +179,7 @@ if (NOT THRIFT_FOUND)
 URL "http://zlib.net/fossils/zlib-1.2.8.tar.gz;
 BUILD_BYPRODUCTS "${ZLIB_STATIC_LIB}"
 ${ZLIB_BUILD_BYPRODUCTS}
+${EP_LOG_OPTIONS}
 CMAKE_ARGS ${ZLIB_CMAKE_ARGS})
 
   set(THRIFT_PREFIX 
"${CMAKE_CURRENT_BINARY_DIR}/thrift_ep/src/thrift_ep-install")
@@ -212,7 +224,10 @@ if (NOT THRIFT_FOUND)
   URL 
https://github.com/lexxmark/winflexbison/releases/download/v.${WINFLEXBISON_VERSION}/win_flex_bison-${WINFLEXBISON_VERSION}.zip
   URL_HASH MD5=a2e979ea9928fbf8567e995e9c0df765
   SOURCE_DIR ${WINFLEXBISON_PREFIX}
-  CONFIGURE_COMMAND "" BUILD_COMMAND "" INSTALL_COMMAND "")
+  CONFIGURE_COMMAND ""
+  BUILD_COMMAND ""
+  INSTALL_COMMAND ""
+  ${EP_LOG_OPTIONS})
 set(THRIFT_DEPENDENCIES ${THRIFT_DEPENDENCIES} winflexbison_ep)
 
 set(THRIFT_CMAKE_ARGS 
"-DFLEX_EXECUTABLE=${WINFLEXBISON_PREFIX}/win_flex.exe"
@@ -229,7 +244,8 @@ if (NOT THRIFT_FOUND)
 URL 
"http://archive.apache.org/dist/thrift/${THRIFT_VERSION}/thrift-${THRIFT_VERSION}.tar.gz;
 BUILD_BYPRODUCTS "${THRIFT_STATIC_LIB}" "${THRIFT_COMPILER}"
 CMAKE_ARGS ${THRIFT_CMAKE_ARGS}
-DEPENDS ${THRIFT_DEPENDENCIES})
+DEPENDS ${THRIFT_DEPENDENCIES}
+${EP_LOG_OPTIONS})
 
   set(THRIFT_VENDORED 1)
 else()
@@ -268,7 +284,7 @@ if(PARQUET_BUILD_TESTS AND NOT IGNORE_OPTIONAL_PACKAGES)
 set(GTEST_CMAKE_ARGS -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
  -DCMAKE_INSTALL_PREFIX=${GTEST_PREFIX}
  -DCMAKE_CXX_FLAGS=${GTEST_CMAKE_CXX_FLAGS})
-
+
 if (MSVC AND NOT PARQUET_USE_STATIC_CRT)
   set(GTEST_CMAKE_ARGS ${GTEST_CMAKE_ARGS} -Dgtest_force_shared_crt=ON)
 endif()
@@ -276,7 +292,8 @@ if(PARQUET_BUILD_TESTS AND NOT IGNORE_OPTIONAL_PACKAGES)
 

[jira] [Commented] (PARQUET-1226) [C++] Fix new build warnings with clang 5.0

2018-02-19 Thread ASF GitHub Bot (JIRA)

[ 
https://issues.apache.org/jira/browse/PARQUET-1226?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16369587#comment-16369587
 ] 

ASF GitHub Bot commented on PARQUET-1226:
-

cpcloud commented on a change in pull request #442: PARQUET-1226: Fixes for 
CHECKIN compiler warning level with clang 5.0
URL: https://github.com/apache/parquet-cpp/pull/442#discussion_r169184645
 
 

 ##
 File path: src/parquet/util/comparison.h
 ##
 @@ -38,15 +38,13 @@ class PARQUET_EXPORT CompareDefault : public Comparator {
  public:
   typedef typename DType::c_type T;
   CompareDefault() {}
-  virtual ~CompareDefault() {}
 
 Review comment:
   Yep.
   
   Found [this SO answer](https://stackoverflow.com/a/27220184/564538), then I 
downloaded the standard and confirmed (the answer says subsection 9, but I 
found it in subsection 8) 
   
   Section 12.4/8 says:
   
   ```
   If a class has a base class with a
   virtual destructor, its destructor (whether user- or implicitly-declared) is 
virtual.
   ```
   
   Coupled with the implicit definitions of a destructor if they are used (and 
not already defined) this means defining an empty virtual destructor is 
unnecessary.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


> [C++] Fix new build warnings with clang 5.0
> ---
>
> Key: PARQUET-1226
> URL: https://issues.apache.org/jira/browse/PARQUET-1226
> Project: Parquet
>  Issue Type: Bug
>  Components: parquet-cpp
>Reporter: Wes McKinney
>Assignee: Wes McKinney
>Priority: Major
> Fix For: cpp-1.5.0
>
>
> Follow-on work since Apache Arrow has migrated to clang 5.0 in ARROW-2117



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)


[jira] [Commented] (PARQUET-1226) [C++] Fix new build warnings with clang 5.0

2018-02-19 Thread ASF GitHub Bot (JIRA)

[ 
https://issues.apache.org/jira/browse/PARQUET-1226?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16369573#comment-16369573
 ] 

ASF GitHub Bot commented on PARQUET-1226:
-

wesm commented on a change in pull request #442: PARQUET-1226: Fixes for 
CHECKIN compiler warning level with clang 5.0
URL: https://github.com/apache/parquet-cpp/pull/442#discussion_r169181576
 
 

 ##
 File path: src/parquet/util/comparison.h
 ##
 @@ -38,15 +38,13 @@ class PARQUET_EXPORT CompareDefault : public Comparator {
  public:
   typedef typename DType::c_type T;
   CompareDefault() {}
-  virtual ~CompareDefault() {}
 
 Review comment:
   @cpcloud I wanted to confirm the the C++ standard says that declaring empty 
virtual dtors in subclasses is unnecessary (per associated comments on this 
recently in Arrow)


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


> [C++] Fix new build warnings with clang 5.0
> ---
>
> Key: PARQUET-1226
> URL: https://issues.apache.org/jira/browse/PARQUET-1226
> Project: Parquet
>  Issue Type: Bug
>  Components: parquet-cpp
>Reporter: Wes McKinney
>Assignee: Wes McKinney
>Priority: Major
> Fix For: cpp-1.5.0
>
>
> Follow-on work since Apache Arrow has migrated to clang 5.0 in ARROW-2117



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)