Skip to content

Commit

Permalink
ARROW-17545: [C++][CI] Mandate C++17 instead of C++11
Browse files Browse the repository at this point in the history
  • Loading branch information
pitrou committed Sep 7, 2022
1 parent c586b9f commit 27f586e
Show file tree
Hide file tree
Showing 70 changed files with 280 additions and 181 deletions.
12 changes: 3 additions & 9 deletions .github/workflows/r.yml
Original file line number Diff line number Diff line change
Expand Up @@ -119,12 +119,13 @@ jobs:
fail-fast: false
matrix:
config:
- { org: "rstudio", image: "r-base", tag: "4.0-centos7" }
- { org: "rhub", image: "debian-gcc-devel", tag: "latest" }
- { org: "rstudio", image: "r-base", tag: "4.0-centos7", devtoolset: "8" }
- { org: "rhub", image: "debian-gcc-devel", tag: "latest", devtoolset: "-1" }
env:
R_ORG: ${{ matrix.config.org }}
R_IMAGE: ${{ matrix.config.image }}
R_TAG: ${{ matrix.config.tag }}
DEVTOOLSET_VERSION: ${{ matrix.config.devtoolset }}
steps:
- name: Checkout Arrow
uses: actions/checkout@v3
Expand Down Expand Up @@ -184,11 +185,6 @@ jobs:
run: |
ci/scripts/ccache_setup.sh
echo "CCACHE_DIR=$(cygpath --absolute --windows ccache)" >> $GITHUB_ENV
# We must enable actions/cache before r-lib/actions/setup-r to ensure
# using system tar instead of tar provided by Rtools.
# We can use tar provided by Rtools when we drop support for Rtools 3.5.
# Because Rtools 4.0 or later has zstd. actions/cache requires zstd
# when tar is GNU tar.
- name: Cache ccache
uses: actions/cache@v2
with:
Expand Down Expand Up @@ -268,8 +264,6 @@ jobs:
with:
r-version: ${{ matrix.config.rversion }}
rtools-version: ${{ matrix.config.rtools }}
# RSPM keeps install times short for 3.6
use-public-rspm: true
Ncpus: 2
- uses: r-lib/actions/setup-r-dependencies@v2
env:
Expand Down
2 changes: 1 addition & 1 deletion c_glib/meson.build
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ project('arrow-glib', 'c', 'cpp',
license: 'Apache-2.0',
default_options: [
'c_std=c99',
'cpp_std=c++11',
'cpp_std=c++17',
])

version = '10.0.0-SNAPSHOT'
Expand Down
2 changes: 1 addition & 1 deletion ci/conan/all/test_package/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,5 @@ find_package(Arrow REQUIRED)

add_executable(${PROJECT_NAME} test_package.cpp)
target_link_libraries(${PROJECT_NAME} arrow::arrow)
target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_11)
target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_17)
target_compile_definitions(${PROJECT_NAME} PRIVATE WITH_JEMALLOC)
2 changes: 1 addition & 1 deletion ci/scripts/cpp_build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ cmake \
-DCMAKE_VERBOSE_MAKEFILE=${CMAKE_VERBOSE_MAKEFILE:-OFF} \
-DCMAKE_C_FLAGS="${CFLAGS:-}" \
-DCMAKE_CXX_FLAGS="${CXXFLAGS:-}" \
-DCMAKE_CXX_STANDARD="${CMAKE_CXX_STANDARD:-11}" \
-DCMAKE_CXX_STANDARD="${CMAKE_CXX_STANDARD:-17}" \
-DCMAKE_INSTALL_LIBDIR=${CMAKE_INSTALL_LIBDIR:-lib} \
-DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX:-${ARROW_HOME}} \
-DCMAKE_UNITY_BUILD=${CMAKE_UNITY_BUILD:-OFF} \
Expand Down
2 changes: 1 addition & 1 deletion ci/scripts/r_docker_configure.sh
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ if [ ${R_CUSTOM_CCACHE} = "true" ]; then
CCACHE=ccache
CC=\$(CCACHE) gcc\$(VER)
CXX=\$(CCACHE) g++\$(VER)
CXX11=\$(CCACHE) g++\$(VER)" >> ~/.R/Makevars
CXX17=\$(CCACHE) g++\$(VER)" >> ~/.R/Makevars

mkdir -p ~/.ccache/
echo "max_size = 5.0G
Expand Down
20 changes: 14 additions & 6 deletions ci/scripts/r_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,21 @@ pushd ${source_dir}

printenv

if [[ "$DEVTOOLSET_VERSION" -gt 0 ]]; then
# enable the devtoolset version to use it
source /opt/rh/devtoolset-$DEVTOOLSET_VERSION/enable

# Build images which require the devtoolset don't have CXX17 variables
# set as the system compiler doesn't support C++17
mkdir -p ~/.R
echo "CC = $(which gcc) -fPIC" >> ~/.R/Makevars
echo "CXX17 = $(which g++) -fPIC" >> ~/.R/Makevars
echo "CXX17STD = -std=c++17" >> ~/.R/Makevars
echo "CXX17FLAGS = ${CXX11FLAGS}" >> ~/.R/Makevars
fi

# Run the nixlibs.R test suite, which is not included in the installed package
${R_BIN} -e 'setwd("tools"); testthat::test_dir(".")'
${R_BIN} -e 'setwd("tools"); testthat::test_dir(".")'

# Before release, we always copy the relevant parts of the cpp source into the
# package. In some CI checks, we will use this version of the source:
Expand Down Expand Up @@ -77,11 +90,6 @@ export ARROW_DEBUG_MEMORY_POOL=trap
export TEXMFCONFIG=/tmp/texmf-config
export TEXMFVAR=/tmp/texmf-var

if [[ "$DEVTOOLSET_VERSION" -gt 0 ]]; then
# enable the devtoolset version to use it
source /opt/rh/devtoolset-$DEVTOOLSET_VERSION/enable
fi

# Make sure we aren't writing to the home dir (CRAN _hates_ this but there is no official check)
BEFORE=$(ls -alh ~/)

Expand Down
6 changes: 3 additions & 3 deletions cpp/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -558,10 +558,10 @@ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${ARROW_CXXFLAGS}")
# C++ specific flags.
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${CXX_COMMON_FLAGS} ${ARROW_CXXFLAGS}")

# Remove --std=c++11 to avoid errors from C compilers
string(REPLACE "-std=c++11" "" CMAKE_C_FLAGS ${CMAKE_C_FLAGS})
# Remove --std=c++17 to avoid errors from C compilers
string(REPLACE "-std=c++17" "" CMAKE_C_FLAGS ${CMAKE_C_FLAGS})

# Add C++-only flags, like -std=c++11
# Add C++-only flags, like -std=c++17
set(CMAKE_CXX_FLAGS "${CXX_ONLY_FLAGS} ${CMAKE_CXX_FLAGS}")

# ASAN / TSAN / UBSAN
Expand Down
3 changes: 0 additions & 3 deletions cpp/build-support/update-flatbuffers.sh
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,6 @@ OUT_DIR="$SOURCE_DIR/generated"
FILES=($(find $FORMAT_DIR -name '*.fbs'))
FILES+=("$SOURCE_DIR/arrow/ipc/feather.fbs")

# add compute ir files
FILES+=($(find "$TOP/experimental/computeir" -name '*.fbs'))

$FLATC --cpp --cpp-std c++11 \
--scoped-enums \
-o "$OUT_DIR" \
Expand Down
18 changes: 10 additions & 8 deletions cpp/cmake_modules/SetupCxxFlags.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -122,12 +122,14 @@ if(NOT DEFINED CMAKE_C_STANDARD)
set(CMAKE_C_STANDARD 11)
endif()

# This ensures that things like c++11 get passed correctly
# This ensures that things like c++17 get passed correctly
if(NOT DEFINED CMAKE_CXX_STANDARD)
set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD 17)
elseif(${CMAKE_CXX_STANDARD} VERSION_LESS 17)
message(FATAL_ERROR "Cannot set a CMAKE_CXX_STANDARD smaller than 17")
endif()

# We require a C++11 compliant compiler
# We require a C++17 compliant compiler
set(CMAKE_CXX_STANDARD_REQUIRED ON)

# ARROW-6848: Do not use GNU (or other CXX) extensions
Expand Down Expand Up @@ -440,11 +442,11 @@ elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" OR CMAKE_CXX_COMPILER_ID STRE
# Don't complain about optimization passes that were not possible
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-pass-failed")

if(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang")
# Depending on the default OSX_DEPLOYMENT_TARGET (< 10.9), libstdc++ may be
# the default standard library which does not support C++11. libc++ is the
# default from 10.9 onward.
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -stdlib=libc++")
# Avoid clang / libc++ error about C++17 aligned allocation on macOS.
# See https://chromium.googlesource.com/chromium/src/+/eee44569858fc650b635779c4e34be5cb0c73186%5E%21/#F0
# for details.
if(APPLE)
set(CXX_ONLY_FLAGS "${CXX_ONLY_FLAGS} -fno-aligned-new")
endif()
endif()

Expand Down
2 changes: 1 addition & 1 deletion cpp/cmake_modules/ThirdpartyToolchain.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -2088,7 +2088,7 @@ macro(build_benchmark)
endif()

if(NOT MSVC)
set(GBENCHMARK_CMAKE_CXX_FLAGS "${EP_CXX_FLAGS} -std=c++11")
set(GBENCHMARK_CMAKE_CXX_FLAGS "${EP_CXX_FLAGS} -std=c++17")
endif()

if(APPLE AND (CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" OR CMAKE_CXX_COMPILER_ID
Expand Down
4 changes: 2 additions & 2 deletions cpp/examples/minimal_build/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ option(ARROW_LINK_SHARED "Link to the Arrow shared library" ON)
find_package(Arrow REQUIRED)

if(NOT DEFINED CMAKE_CXX_STANDARD)
set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD 17)
endif()

# We require a C++11 compliant compiler
# We require a C++17 compliant compiler
set(CMAKE_CXX_STANDARD_REQUIRED ON)

if(NOT DEFINED CMAKE_BUILD_TYPE)
Expand Down
6 changes: 3 additions & 3 deletions cpp/examples/parquet/parquet_arrow/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,12 @@ include(GNUInstallDirs)

option(PARQUET_LINK_SHARED "Link to the Parquet shared library" ON)

# This ensures that things like gnu++11 get passed correctly
# This ensures that things like -std=gnu++... get passed correctly
if(NOT DEFINED CMAKE_CXX_STANDARD)
set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD 17)
endif()

# We require a C++11 compliant compiler
# We require a C++17 compliant compiler
set(CMAKE_CXX_STANDARD_REQUIRED ON)

# Look for installed packages the system
Expand Down
2 changes: 2 additions & 0 deletions cpp/src/arrow/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -636,6 +636,8 @@ endif()

foreach(LIB_TARGET ${ARROW_LIBRARIES})
target_compile_definitions(${LIB_TARGET} PRIVATE ARROW_EXPORTING)
# C++17 is required to compile against Arrow C++ headers and libraries
target_compile_features(${LIB_TARGET} PUBLIC cxx_std_17)
endforeach()

if(ARROW_WITH_BACKTRACE)
Expand Down
3 changes: 2 additions & 1 deletion cpp/src/arrow/dataset/file_csv.h
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,8 @@ class ARROW_DS_EXPORT CsvFileWriteOptions : public FileWriteOptions {
std::shared_ptr<csv::WriteOptions> write_options;

protected:
using FileWriteOptions::FileWriteOptions;
explicit CsvFileWriteOptions(std::shared_ptr<FileFormat> format)
: FileWriteOptions(std::move(format)) {}

friend class CsvFileFormat;
};
Expand Down
3 changes: 2 additions & 1 deletion cpp/src/arrow/dataset/file_ipc.h
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@ class ARROW_DS_EXPORT IpcFileWriteOptions : public FileWriteOptions {
std::shared_ptr<const KeyValueMetadata> metadata;

protected:
using FileWriteOptions::FileWriteOptions;
explicit IpcFileWriteOptions(std::shared_ptr<FileFormat> format)
: FileWriteOptions(std::move(format)) {}

friend class IpcFileFormat;
};
Expand Down
3 changes: 2 additions & 1 deletion cpp/src/arrow/dataset/file_parquet.h
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,8 @@ class ARROW_DS_EXPORT ParquetFileWriteOptions : public FileWriteOptions {
std::shared_ptr<parquet::ArrowWriterProperties> arrow_writer_properties;

protected:
using FileWriteOptions::FileWriteOptions;
explicit ParquetFileWriteOptions(std::shared_ptr<FileFormat> format)
: FileWriteOptions(std::move(format)) {}

friend class ParquetFileFormat;
};
Expand Down
4 changes: 4 additions & 0 deletions cpp/src/arrow/memory_pool.cc
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,8 @@ class DebugAllocator {
*out = memory_pool::internal::kZeroSizeArea;
} else {
ARROW_ASSIGN_OR_RAISE(int64_t raw_size, RawSize(size));
DCHECK(raw_size > size) << "bug in raw size computation: " << raw_size
<< " for size " << size;
RETURN_NOT_OK(WrappedAllocator::AllocateAligned(raw_size, out));
InitAllocatedArea(*out, size);
}
Expand All @@ -250,6 +252,8 @@ class DebugAllocator {
return Status::OK();
}
ARROW_ASSIGN_OR_RAISE(int64_t raw_new_size, RawSize(new_size));
DCHECK(raw_new_size > new_size)
<< "bug in raw size computation: " << raw_new_size << " for size " << new_size;
RETURN_NOT_OK(
WrappedAllocator::ReallocateAligned(old_size + kOverhead, raw_new_size, ptr));
InitAllocatedArea(*ptr, new_size);
Expand Down
19 changes: 19 additions & 0 deletions cpp/src/arrow/util/aligned_storage.h
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,26 @@ class AlignedStorage {
}

private:
#if !defined(__clang__) && defined(__GNUC__) && defined(__i386__)
// Workaround for GCC bug on i386:
// alignof(int64 | float64) can give different results depending on the
// compilation context, leading to internal ABI mismatch manifesting
// in incorrect propagation of Result<int64 | float64> between
// compilation units.
// (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88115)
static constexpr size_t alignment() {
if (std::is_integral_v<T> && sizeof(T) == 8) {
return 4;
} else if (std::is_floating_point_v<T> && sizeof(T) == 8) {
return 4;
}
return alignof(T);
}

typename std::aligned_storage<sizeof(T), alignment()>::type data_;
#else
typename std::aligned_storage<sizeof(T), alignof(T)>::type data_;
#endif
};

} // namespace internal
Expand Down
55 changes: 55 additions & 0 deletions cpp/src/arrow/util/int_util_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#include <algorithm>
#include <cstdint>
#include <limits>
#include <random>
#include <string>
#include <utility>
Expand Down Expand Up @@ -594,5 +595,59 @@ TEST(CheckIntegersInRange, UnsignedInts) {
CheckInRangeFails(uint64(), "[0, 10000000000, 10000000000]", "[0, 9999999999]");
}

template <typename T>
class TestAddWithOverflow : public ::testing::Test {
public:
void CheckOk(T a, T b, T expected_result = {}) {
ARROW_SCOPED_TRACE("a = ", a, ", b = ", b);
T result;
ASSERT_FALSE(AddWithOverflow(a, b, &result));
ASSERT_EQ(result, expected_result);
}

void CheckOverflow(T a, T b) {
ARROW_SCOPED_TRACE("a = ", a, ", b = ", b);
T result;
ASSERT_TRUE(AddWithOverflow(a, b, &result));
}
};

using SignedIntegerTypes = ::testing::Types<int8_t, int16_t, int32_t, int64_t>;

TYPED_TEST_SUITE(TestAddWithOverflow, SignedIntegerTypes);

TYPED_TEST(TestAddWithOverflow, Basics) {
using T = TypeParam;

const T almost_max = std::numeric_limits<T>::max() - T{2};
const T almost_min = std::numeric_limits<T>::min() + T{2};

this->CheckOk(T{1}, T{2}, T{3});
this->CheckOk(T{-1}, T{2}, T{1});
this->CheckOk(T{-1}, T{-2}, T{-3});

this->CheckOk(almost_min, T{0}, almost_min);
this->CheckOk(almost_min, T{-2}, almost_min - T{2});
this->CheckOk(almost_min, T{1}, almost_min + T{1});
this->CheckOverflow(almost_min, T{-3});
this->CheckOverflow(almost_min, almost_min);

this->CheckOk(almost_max, T{0}, almost_max);
this->CheckOk(almost_max, T{2}, almost_max + T{2});
this->CheckOk(almost_max, T{-1}, almost_max - T{1});
this->CheckOverflow(almost_max, T{3});
this->CheckOverflow(almost_max, almost_max);

// In 2's complement, almost_min == - almost_max - 1
this->CheckOk(almost_min, almost_max, T{-1});
this->CheckOk(almost_max, almost_min, T{-1});
this->CheckOk(almost_min - T{1}, almost_max, T{-2});
this->CheckOk(almost_min + T{1}, almost_max, T{0});
this->CheckOk(almost_min + T{2}, almost_max, T{1});
this->CheckOk(almost_min, almost_max - T{1}, T{-2});
this->CheckOk(almost_min, almost_max + T{1}, T{0});
this->CheckOk(almost_min, almost_max + T{2}, T{1});
}

} // namespace internal
} // namespace arrow
2 changes: 1 addition & 1 deletion cpp/src/arrow/util/launder.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
namespace arrow {
namespace internal {

#if __cplusplus >= 201703L
#if __cpp_lib_launder
using std::launder;
#else
template <class T>
Expand Down
11 changes: 1 addition & 10 deletions cpp/src/gandiva/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,7 @@ add_dependencies(gandiva-all gandiva gandiva-tests gandiva-benchmarks)

find_package(LLVMAlt REQUIRED)

if(LLVM_VERSION_MAJOR LESS "10")
set(GANDIVA_CXX_STANDARD ${CMAKE_CXX_STANDARD})
else()
# LLVM 10 or later requires C++ 14
if(CMAKE_CXX_STANDARD LESS 14)
set(GANDIVA_CXX_STANDARD 14)
else()
set(GANDIVA_CXX_STANDARD ${CMAKE_CXX_STANDARD})
endif()
endif()
set(GANDIVA_CXX_STANDARD ${CMAKE_CXX_STANDARD})

add_definitions(-DGANDIVA_LLVM_VERSION=${LLVM_VERSION_MAJOR})

Expand Down
4 changes: 2 additions & 2 deletions cpp/src/gandiva/precompiled/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,10 @@ if(MSVC)
else()
message(FATAL_ERROR "Unsupported MSVC_VERSION=${MSVC_VERSION}")
endif()
set(PLATFORM_CLANG_OPTIONS -std=c++14 -fms-compatibility
set(PLATFORM_CLANG_OPTIONS -std=c++17 -fms-compatibility
-fms-compatibility-version=${FMS_COMPATIBILITY})
else()
set(PLATFORM_CLANG_OPTIONS -std=c++11)
set(PLATFORM_CLANG_OPTIONS -std=c++17)
endif()

# Create bitcode for each of the source files.
Expand Down
Loading

0 comments on commit 27f586e

Please sign in to comment.