Skip to content

Commit

Permalink
Drop support for Clang 13 (ad-freiburg#927)
Browse files Browse the repository at this point in the history
QLever builds quite heavily on C++20 features, and clang++-13 has a few hiccups with that standard, so that we had to disable warnings at several places in our code.
  • Loading branch information
joka921 authored Mar 27, 2023
1 parent e021955 commit a454f3a
Show file tree
Hide file tree
Showing 10 changed files with 25 additions and 86 deletions.
11 changes: 1 addition & 10 deletions .github/workflows/native-build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,10 @@ jobs:
fail-fast: false
matrix:
compiler: [gcc, clang]
compiler-version: [11, 12, 13, 14, 15, 16]
compiler-version: [11, 12, 14, 15, 16]
warnings: [ "-Wall -Wextra -Werror " ]
build-type: [Release]
exclude:
- compiler: gcc
compiler-version: 13
- compiler: gcc
compiler-version: 14
- compiler: gcc
Expand Down Expand Up @@ -58,13 +56,6 @@ jobs:
- name: Install gcc 12
run : sudo add-apt-repository ppa:ubuntu-toolchain-r/test && sudo apt update && sudo apt install -y gcc-12 g++-12
if : matrix.compiler == 'gcc' && matrix.compiler-version == 12
- name: Install clang 13
run: sudo apt install clang-13
if : matrix.compiler == 'clang' && matrix.compiler-version == 13
# Currently not needed, because we are not building against libc++
#- name: Install libc++-13
# run : sudo apt install -y libunwind-13-dev libc++abi-13-dev libc++-13-dev
# if: matrix.compiler == 'clang13'
- name: Install clang 14
run : sudo apt install clang-14
if : matrix.compiler == 'clang' && matrix.compiler-version == 14
Expand Down
22 changes: 16 additions & 6 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,22 @@ set(CMAKE_CXX_STANDARD_REQUIRED ON)

# Coroutines require an additional compiler flag that is called differently
# on clang and g++
include(CheckCXXCompilerFlag)
check_cxx_compiler_flag(-fcoroutines HAS_COROUTINES)
if (HAS_COROUTINES)
add_compile_options(-fcoroutines)
elseif((CMAKE_CXX_COMPILER_ID STREQUAL "Clang") AND (CMAKE_CXX_COMPILER_VERSION VERSION_LESS "16.0.0"))
add_compile_options(-fcoroutines-ts)
if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
if(CMAKE_CXX_COMPILER_VERSION VERSION_LESS "11.0.0")
MESSAGE(FATAL_ERROR, "G++ versions older than 11.0 are not supported by QLever")
else()
add_compile_options(-fcoroutines)
endif()

elseif(CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
if(CMAKE_CXX_COMPILER_VERSION VERSION_LESS "14.0.0")
MESSAGE(FATAL_ERROR, "Clang++ versions older than 14.0 are not supported by QLever")
elseif(CMAKE_CXX_COMPILER_VERSION VERSION_LESS "16.0.0")
add_compile_options(-fcoroutines-ts)
# starting from Clang 16, coroutines are enabled by default for C++ 20
endif()
else()
MESSAGE(FATAL_ERROR, "QLever currently only supports the G++ or LLVM-Clang++ compilers")
endif()

## Build targets for address sanitizer
Expand Down
3 changes: 0 additions & 3 deletions src/engine/CallFixedSize.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

#include "global/Constants.h"
#include "util/ConstexprUtils.h"
#include "util/DisableWarningsClang13.h"
#include "util/Exception.h"
#include "util/Forward.h"

Expand Down Expand Up @@ -72,9 +71,7 @@ auto callLambdaForIntArray(std::array<Int, NumValues> array, auto&& lambda, auto
// Lambda: If the compile time parameter `I` and the runtime parameter `array`
// are equal, then call the `lambda` with `I` as a template parameter and
// store the result in `result` (unless it is `void`).
DISABLE_WARNINGS_CLANG_13
auto applyIf = [&]<ArrayType Array>() mutable {
ENABLE_WARNINGS_CLANG_13
if (array == Array) {
if constexpr (resultIsVoid) {
applyOnIntegerSequence(ad_utility::toIntegerSequence<Array>());
Expand Down
2 changes: 0 additions & 2 deletions src/engine/GroupBy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -485,10 +485,8 @@ bool GroupBy::computeGroupByForFullIndexScan(ResultTable* result) {
// A nested lambda that computes the actual result. The outer lambda is
// templated on the number of columns (1 or 2) and will be passed to
// `callFixedSize`.
DISABLE_WARNINGS_CLANG_13
auto doComputationForNumberOfColumns = [&]<int NUM_COLS>(
IdTable* idTable) mutable {
ENABLE_WARNINGS_CLANG_13
// TODO<joka921> The `resultTypes` are not really in use, but if they
// are not present, segfaults might occur in upstream `Operation`s.
auto ignoredRanges =
Expand Down
2 changes: 0 additions & 2 deletions src/engine/OrderBy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -132,11 +132,9 @@ void OrderBy::computeResult(ResultTable* result) {
// We cannot use the `CALL_FIXED_SIZE` macro here because the `sort` function
// is templated not only on the integer `I` (which the `callFixedSize`
// function deals with) but also on the `comparison`.
DISABLE_WARNINGS_CLANG_13
ad_utility::callFixedSize(width, [&result, &comparison]<int I>() {
Engine::sort<I>(&result->_idTable, comparison);
});
ENABLE_WARNINGS_CLANG_13
result->_sortedBy = resultSortedOn();

LOG(DEBUG) << "OrderBy result computation done." << endl;
Expand Down
2 changes: 0 additions & 2 deletions src/engine/Sort.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,10 @@ namespace {
// TODO<joka921, future compilers> Check if this problem goes away in future
// compiler versions as soon as we don't support Clang 13 anymore.
void callFixedSizeForSort(auto& idTable, auto comparison) {
DISABLE_WARNINGS_CLANG_13
ad_utility::callFixedSize(idTable.numColumns(),
[&idTable, comparison]<int I>() {
Engine::sort<I>(&idTable, comparison);
});
ENABLE_WARNINGS_CLANG_13
}

// The actual implementation of sorting an `IdTable` according to the
Expand Down
37 changes: 0 additions & 37 deletions src/util/DisableWarningsClang13.h

This file was deleted.

25 changes: 8 additions & 17 deletions test/FTSAlgorithmsTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
#include "./util/IdTestHelpers.h"
#include "engine/CallFixedSize.h"
#include "index/FTSAlgorithms.h"
#include "util/DisableWarningsClang13.h"

using namespace ad_utility::testing;

Expand Down Expand Up @@ -296,13 +295,11 @@ TEST(FTSAlgorithmsTest, aggScoresAndTakeTopContextTest) {

// In the following there are many similar calls to `callFixedSize`.
// We use a lambda to reduce code duplication.
DISABLE_WARNINGS_CLANG_13
auto callFixed = [](int width, auto&&... args) {
ad_utility::callFixedSize(width, [&]<int WIDTH>() {
FTSAlgorithms::aggScoresAndTakeTopContext<WIDTH>(AD_FWD(args)...);
});
};
ENABLE_WARNINGS_CLANG_13

callFixed(width, cids, eids, scores, &result);

Expand Down Expand Up @@ -345,10 +342,8 @@ TEST(FTSAlgorithmsTest, aggScoresAndTakeTopContextTest) {
eids.push_back(V(0));
scores.push_back(10);

DISABLE_WARNINGS_CLANG_13
ad_utility::callFixedSize(
width, [&cids, &eids, &scores, &result]<int WIDTH>() mutable {
ENABLE_WARNINGS_CLANG_13
FTSAlgorithms::aggScoresAndTakeTopContext<WIDTH>(cids, eids, scores,
&result);
});
Expand Down Expand Up @@ -458,12 +453,10 @@ TEST(FTSAlgorithmsTest, appendCrossProductWithTwoW1Test) {

TEST(FTSAlgorithmsTest, multVarsAggScoresAndTakeTopKContexts) {
auto callFixed = [](int width, auto&&... args) {
ad_utility::callFixedSize(
DISABLE_WARNINGS_CLANG_13 width, [&]<int WIDTH>() mutable {
ENABLE_WARNINGS_CLANG_13
FTSAlgorithms::multVarsAggScoresAndTakeTopKContexts<WIDTH>(
AD_FWD(args)...);
});
ad_utility::callFixedSize(width, [&]<int WIDTH>() mutable {
FTSAlgorithms::multVarsAggScoresAndTakeTopKContexts<WIDTH>(
AD_FWD(args)...);
});
};

vector<TextRecordIndex> cids;
Expand Down Expand Up @@ -683,12 +676,10 @@ TEST(FTSAlgorithmsTest, multVarsFilterAggScoresAndTakeTopKContexts) {
// to use an explicit lambda to pass to `callFixedSize`.

auto test = [&](int width, auto&&... args) {
ad_utility::callFixedSize(
DISABLE_WARNINGS_CLANG_13 width, [&]<int V>() mutable {
ENABLE_WARNINGS_CLANG_13
FTSAlgorithms::multVarsFilterAggScoresAndTakeTopKContexts<V>(
AD_FWD(args)...);
});
ad_utility::callFixedSize(width, [&]<int V>() mutable {
FTSAlgorithms::multVarsFilterAggScoresAndTakeTopKContexts<V>(
AD_FWD(args)...);
});
};
test(width, cids, eids, scores, fMap1, nofVars, k, &resW4);
ASSERT_EQ(0u, resW4.size());
Expand Down
4 changes: 0 additions & 4 deletions test/JoinTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -127,14 +127,10 @@ void runTestCasesForAllJoinAlgorithms(
// All normal join algorithm defined as lambda functions for easier handing
// over to helper functions.
Join J{Join::InvalidOnlyForTestingJoinTag{}};
DISABLE_WARNINGS_CLANG_13
auto hashJoinLambda = [&J]<int A, int B, int C>(auto&&... args) {
ENABLE_WARNINGS_CLANG_13
return J.hashJoin(AD_FWD(args)...);
};
DISABLE_WARNINGS_CLANG_13
auto joinLambda = [&J]<int A, int B, int C>(auto&&... args) {
ENABLE_WARNINGS_CLANG_13
return J.join<A, B, C>(AD_FWD(args)...);
};

Expand Down
3 changes: 0 additions & 3 deletions toolchains/clang13.cmake

This file was deleted.

0 comments on commit a454f3a

Please sign in to comment.