Skip to content

Commit 29143ef

Browse files
kevinwilfongfacebook-github-bot
authored andcommitted
C++20 compatibility (#1479)
Summary: This diff fixes a few incompatibility issues with C++ 20. 1) fmt::format checks at compile time that the format string is constexpr or implicitly constructed from a string literal. There are a couple violations, wrapping them in fmt::runtime is less performant than making them constexpr, but they're in tests/uninteresting parts of benchmarks so it's fine. 2) A char8_t type was added for UTF-8 strings, this breaks implicit conversion of string literals using the u8 literal to strings. This affected a few tests, but the u8 literal was unnecessary 3) Ambiguous equality expression warnings when comparing two different types. With an expression like a == b the compiler will look up the equality function for a and the equality function for b and issue a warning if they're different functions. We were explicitly doing this to test the inequality of different types in a few tests, explicitly calling the equality operator on the object produces the original behavior. I also had to update the fmt dependency used in the setup-circleci.sh script. The one available in dnf appears to be out of date, so I pulled the code from GitHub and built it, using the same version and in the same manner the other setup scripts prepare fmt. Pull Request resolved: #1479 Reviewed By: kgpai Differential Revision: D35939375 Pulled By: kevinwilfong fbshipit-source-id: 3e9d8eb0bd63b6e911bb8eaf29ff37bef2a00097
1 parent b99ce73 commit 29143ef

File tree

13 files changed

+68
-67
lines changed

13 files changed

+68
-67
lines changed

.circleci/config.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ workflows:
5555
executors:
5656
build:
5757
docker:
58-
- image : prestocpp/velox-circleci:kpai-20220401
58+
- image : prestocpp/velox-circleci:kevinwilfong-20220426
5959
resource_class: 2xlarge
6060
environment:
6161
CC: /opt/rh/gcc-toolset-9/root/bin/gcc

scripts/setup-circleci.sh

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ dnf_install epel-release dnf-plugins-core # For ccache, ninja
2828
dnf config-manager --set-enabled powertools
2929
dnf_install ninja-build ccache gcc-toolset-9 git wget which libevent-devel \
3030
openssl-devel re2-devel libzstd-devel lz4-devel double-conversion-devel \
31-
protobuf-devel fmt-devel libdwarf-devel curl-devel
31+
protobuf-devel libdwarf-devel curl-devel
3232

3333
dnf remove -y gflags
3434

@@ -64,6 +64,7 @@ wget_and_untar http://www.oberhumer.com/opensource/lzo/download/lzo-2.10.tar.gz
6464
wget_and_untar https://boostorg.jfrog.io/artifactory/main/release/1.72.0/source/boost_1_72_0.tar.gz boost &
6565
wget_and_untar https://github.com/google/snappy/archive/1.1.8.tar.gz snappy &
6666
wget_and_untar https://github.com/facebook/folly/archive/v2022.03.14.00.tar.gz folly &
67+
wget_and_untar https://github.com/fmtlib/fmt/archive/8.0.0.tar.gz fmt &
6768
# wget_and_untar https://github.com/ericniebler/range-v3/archive/0.11.0.tar.gz ranges-v3 &
6869

6970
wait # For cmake and source downloads to complete.
@@ -85,6 +86,8 @@ wait # For cmake and source downloads to complete.
8586
cmake_install gflags -DBUILD_SHARED_LIBS=ON -DBUILD_STATIC_LIBS=ON -DBUILD_gflags_LIB=ON -DLIB_SUFFIX=64 -DCMAKE_INSTALL_PREFIX:PATH=/usr
8687
cmake_install glog -DBUILD_SHARED_LIBS=ON -DCMAKE_INSTALL_PREFIX:PATH=/usr
8788
cmake_install snappy -DSNAPPY_BUILD_TESTS=OFF
89+
# folly depends on fmt so this needs to come first.
90+
cmake_install fmt -DFMT_TEST=OFF
8891
cmake_install folly
8992
# cmake_install ranges-v3
9093

velox/common/base/tests/ExceptionTest.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -354,7 +354,7 @@ TEST(ExceptionTest, notNull) {
354354
TEST(ExceptionTest, expressionString) {
355355
size_t i = 1;
356356
size_t j = 100;
357-
std::string msgTemplate =
357+
constexpr auto msgTemplate =
358358
"Exception: VeloxRuntimeError"
359359
"\nError Source: RUNTIME"
360360
"\nError Code: INVALID_STATE"

velox/dwio/parquet/tests/ParquetTpchTest.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ class ParquetTpchTest : public testing::Test {
6969
fs::create_directory(tableDirectory);
7070
auto filePath = fmt::format("{}/file.parquet", tableDirectory);
7171
auto query = fmt::format(
72-
duckDbParquetWriteSQL_.at(tableName),
72+
fmt::runtime(duckDbParquetWriteSQL_.at(tableName)),
7373
tableName,
7474
filePath,
7575
kRowGroupSize);

velox/functions/lib/tests/Re2FunctionsTest.cpp

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -380,16 +380,13 @@ TEST_F(Re2FunctionsTest, likePattern) {
380380

381381
EXPECT_EQ(
382382
like(
383-
u8"\u4FE1\u5FF5 \u7231 \u5E0C\u671B \u2028 abc",
384-
u8"\u4FE1\u5FF5 \u7231%"),
383+
"\u4FE1\u5FF5 \u7231 \u5E0C\u671B \u2028 abc",
384+
"\u4FE1\u5FF5 \u7231%"),
385385
true);
386386
EXPECT_EQ(
387-
like(u8"\u4FE1\u5FF5 \u7231 \u5E0C\u671B \u2028 ", u8"\u4FE1%\u7231%"),
388-
true);
387+
like("\u4FE1\u5FF5 \u7231 \u5E0C\u671B \u2028 ", "\u4FE1%\u7231%"), true);
389388
EXPECT_EQ(
390-
like(
391-
u8"\u4FE1\u5FF5 \u7231 \u5E0C\u671B \u2028 ",
392-
u8"\u7231\u4FE1%\u7231%"),
389+
like("\u4FE1\u5FF5 \u7231 \u5E0C\u671B \u2028 ", "\u7231\u4FE1%\u7231%"),
393390
false);
394391

395392
EXPECT_EQ(like("abc", "MEDIUM POLISHED%"), false);

velox/functions/prestosql/aggregates/tests/ChecksumAggregateTest.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ TEST_F(ChecksumAggregateTest, varchars) {
152152
assertSingleGroupChecksum<StringView>({{}}, "h8rrhbF5N54=");
153153
assertSingleGroupChecksum<StringView>({"abcd"_sv}, "lGFxgnIYgPw=");
154154
assertSingleGroupChecksum<StringView>(
155-
{u8"Thanks \u0020\u007F"_sv}, "oEh7YyEV+dM=");
155+
{"Thanks \u0020\u007F"_sv}, "oEh7YyEV+dM=");
156156
}
157157

158158
TEST_F(ChecksumAggregateTest, arrays) {

velox/functions/prestosql/aggregates/tests/PrestoHasherTest.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,7 @@ TEST_F(PrestoHasherTest, floats) {
211211

212212
TEST_F(PrestoHasherTest, varchars) {
213213
assertHash<StringView>(
214-
{"abcd"_sv, ""_sv, std::nullopt, u8"Thanks \u0020\u007F"_sv},
214+
{"abcd"_sv, ""_sv, std::nullopt, "Thanks \u0020\u007F"_sv},
215215
{-2449070131962342708, -1205034819632174695, 0, 2911531567394159200});
216216
}
217217

velox/functions/prestosql/aggregates/tests/VarianceAggregationTest.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ namespace {
2424

2525
// Replace the given query's placeholders '{0}' with the given aggregation name.
2626
std::string genAggrQuery(const char* query, const char* aggrName) {
27-
return fmt::format(query, aggrName);
27+
return fmt::format(fmt::runtime(query), aggrName);
2828
}
2929

3030
// Helper generates aggregation over column string.

velox/functions/prestosql/benchmarks/URLBenchmark.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -207,8 +207,8 @@ class UrlBenchmark : public functions::test::FunctionBenchmarkBase {
207207
: vectorMaker_.rowVector({vectorUrls});
208208

209209
auto queryString = isParameter ? "{}(c0, c1)" : "{}(c0)";
210-
auto exprSet =
211-
compileExpression(fmt::format(queryString, fnName), rowVector->type());
210+
auto exprSet = compileExpression(
211+
fmt::format(fmt::runtime(queryString), fnName), rowVector->type());
212212

213213
suspender.dismiss();
214214

velox/functions/prestosql/tests/JsonExtractScalarTest.cpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -65,15 +65,15 @@ TEST_F(JsonExtractScalarTest, simple) {
6565
TEST_F(JsonExtractScalarTest, utf8) {
6666
EXPECT_EQ(
6767
json_extract_scalar(R"({"k1":"I \u2665 UTF-8"})", "$.k1"),
68-
u8"I \u2665 UTF-8");
68+
"I \u2665 UTF-8");
6969
EXPECT_EQ(
70-
json_extract_scalar(u8"{\"k1\":\"I \u2665 UTF-8\"}", "$.k1"),
71-
u8"I \u2665 UTF-8");
70+
json_extract_scalar("{\"k1\":\"I \u2665 UTF-8\"}", "$.k1"),
71+
"I \u2665 UTF-8");
7272

7373
EXPECT_EQ(
7474
json_extract_scalar(
75-
u8"{\"k1\":\"I \U0001D11E playing in G-clef\"}", "$.k1"),
76-
u8"I \U0001D11E playing in G-clef");
75+
"{\"k1\":\"I \U0001D11E playing in G-clef\"}", "$.k1"),
76+
"I \U0001D11E playing in G-clef");
7777
}
7878

7979
TEST_F(JsonExtractScalarTest, invalidPath) {

0 commit comments

Comments
 (0)