Skip to content
This repository was archived by the owner on Mar 22, 2023. It is now read-only.

[array] add c++20 initialization_list constructor#1158

Merged
igchor merged 5 commits intopmem:masterfrom
karczex:array-cpp20
Jul 19, 2021
Merged

[array] add c++20 initialization_list constructor#1158
igchor merged 5 commits intopmem:masterfrom
karczex:array-cpp20

Conversation

@karczex
Copy link
Copy Markdown

@karczex karczex commented Jul 14, 2021

Since C++20 classes with explicitly defaulted constructors are no more
aggregates, so constructor for initialization_list is needed.


This change is Reviewable

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 14, 2021

Codecov Report

Merging #1158 (afa93e0) into master (0ddba32) will increase coverage by 0.01%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1158      +/-   ##
==========================================
+ Coverage   94.05%   94.06%   +0.01%     
==========================================
  Files          52       52              
  Lines        5163     5124      -39     
==========================================
- Hits         4856     4820      -36     
+ Misses        307      304       -3     
Flag Coverage Δ
tests_clang_debug_cpp17 93.63% <50.00%> (+0.08%) ⬆️
tests_gcc_debug 90.66% <100.00%> (-0.14%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
include/libpmemobj++/container/array.hpp 98.07% <50.00%> (+0.42%) ⬆️
include/libpmemobj++/experimental/radix_tree.hpp 98.46% <0.00%> (+0.14%) ⬆️
include/libpmemobj++/detail/life.hpp 97.43% <0.00%> (+0.21%) ⬆️
include/libpmemobj++/transaction.hpp 82.35% <0.00%> (+0.84%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ddba32...afa93e0. Read the comment docs.

Copy link
Copy Markdown
Contributor

@igchor igchor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In future, we might think about implementing something like was proposed here: https://stackoverflow.com/questions/38932089/can-i-initialize-an-array-using-the-stdinitializer-list-instead-of-brace-enclo to avoid the limitations of std::initializer_list

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @karczex)


include/libpmemobj++/container/array.hpp, line 49 at r1 (raw file):

 * aggregate since C++20.
 *
 * @note Since C++20 pmem::obj::array cannot be used for types with deleted

I don't think this is true - you can use array in that case, you just cannot initialize those values in a ctor. You can just do (if T has a default ctor):

pmem::obj::array<T, 3> arr;
arr[0] = ...;

include/libpmemobj++/container/array.hpp, line 52 at r1 (raw file):

 * copy constructor. This may change in the future.
 *
 * @note Since C++20 pmem::obj::array cannot be used for const types. This may

Well, it can, but you can't use any ctor different than a default one.

Copy link
Copy Markdown
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 6 files reviewed, 6 unresolved discussions (waiting on @karczex)


include/libpmemobj++/container/array.hpp, line 46 at r1 (raw file):

 * to a transaction while traversing.
 *
 * @note Accordingly to changes in C++ standard, pmem::obj::array is not an

According to


tests/CMakeLists.txt, line 357 at r1 (raw file):

		add_test_generic(NAME array_slice_pmreorder CASE 1 TRACERS none SCRIPT array/array_slice_pmreorder_1.cmake)
	endif()
	if(CXX_STANDARD GREATER_EQUAL 17)

GREATER_EQUAL is delivered in CMake >= 3.7, so please just use CXX_STANDARD GREATER 14


tests/array/array_is_aggregate.cpp, line 8 at r1 (raw file):

/* Test which checks if array behave in the same way as any structure with
 * defaulted constructor - accordingly to used c++ standard */

according to


tests/external/CMakeLists.txt, line 108 at r1 (raw file):

	if(CXX_STANDARD LESS_EQUAL 17)

. (LESS 20)

Copy link
Copy Markdown
Contributor

@KFilipek KFilipek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @karczex and @lukaszstolarczuk)


include/libpmemobj++/container/array.hpp, line 15 at r1 (raw file):

#include <functional>

#include <initializer_list>

one block up:

#include <algorithm>
#include <functional>
#include <initializer_list>

tests/CMakeLists.txt, line 357 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

GREATER_EQUAL is delivered in CMake >= 3.7, so please just use CXX_STANDARD GREATER 14

or update README.md, on every supported OS this PR was built successfully


tests/array/array_is_aggregate.cpp, line 4 at r1 (raw file):

#include "unittest.hpp"
#include <libpmemobj++/container/array.hpp>

suparate this into 2 blocks


tests/array/array_is_aggregate.cpp, line 21 at r1 (raw file):

		std::is_aggregate<struct_with_defaulted_constructor>::value;

	UT_ASSERTeq(array_is_aggregate, cpp_standard_support);

@igchor is static_assert here better than UT_ASSERTeq?


tests/external/CMakeLists.txt, line 107 at r1 (raw file):

	add_test_generic(NAME array_get_const TRACERS none)

2 lines instead of 1


tests/external/libcxx/array/array.cons/implicit_copy_const.pass.cpp, line 10 at r1 (raw file):

//===----------------------------------------------------------------------===//
//
// Copyright 2018-2021, Intel Corporation

If it's new file it should contain only 2021


tests/external/libcxx/array/array.cons/implicit_copy_const.pass.cpp, line 54 at r1 (raw file):

	run()
	{
		((void)c2);

(void)c2; should be sufficient


tests/external/libcxx/array/array.cons/implicit_copy_const.pass.cpp, line 67 at r1 (raw file):

	run()
	{
		((void)c2);

.

Copy link
Copy Markdown
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @karczex and @KFilipek)


tests/CMakeLists.txt, line 357 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

or update README.md, on every supported OS this PR was built successfully

it may break on OSes like RHEL - and what would be the gain...? I'd say, it's better to use as low version of CMake as possible 😉


tests/external/libcxx/array/array.cons/implicit_copy_const.pass.cpp, line 10 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

If it's new file it should contain only 2021

nope, as usual - we copy-pasted the content from previously existing file - hence both dates

Copy link
Copy Markdown
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Reviewed 2 of 6 files at r1, 8 of 8 files at r2.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @karczex and @KFilipek)


utils/docker/run-build.sh, line 217 at r2 (raw file):

	printf "\n$(tput setaf 1)$(tput setab 7)BUILD ${FUNCNAME[0]} START$(tput sgr 0)\n"
	mkdir build && cd build

we may add here the check for support in CMake (like here: https://github.com/pmem/pmemkv/blob/master/utils/docker/run-build.sh#L168), but it will work on most OSes...

Copy link
Copy Markdown
Contributor

@KFilipek KFilipek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @karczex)


tests/CMakeLists.txt, line 357 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

it may break on OSes like RHEL - and what would be the gain...? I'd say, it's better to use as low version of CMake as possible 😉

Indeed

Copy link
Copy Markdown
Contributor

@igchor igchor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @karczex and @KFilipek)


tests/array/array_is_aggregate.cpp, line 21 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

@igchor is static_assert here better than UT_ASSERTeq?

I think it's fine both ways.


tests/external/CMakeLists.txt, line 107 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

2 lines instead of 1

Done.


tests/external/libcxx/array/array.cons/implicit_copy_const.pass.cpp, line 54 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

(void)c2; should be sufficient

Done.


tests/external/libcxx/array/array.cons/implicit_copy_const.pass.cpp, line 67 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

.

.


utils/docker/run-build.sh, line 217 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

we may add here the check for support in CMake (like here: https://github.com/pmem/pmemkv/blob/master/utils/docker/run-build.sh#L168), but it will work on most OSes...

Done.

Copy link
Copy Markdown
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @karczex and @KFilipek)


include/libpmemobj++/container/array.hpp, line 116 at r3 (raw file):

	 *
	 * It provides similar initialization semantics for
	 * C++20 (where pmem::obj::array is not longer an aggregate).

no longer


tests/external/libcxx/array/array.cons/implicit_copy_const.pass.cpp, line 67 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

.

nope..?


tests/external/libcxx/array/array.cons/initializer_list.pass.cpp, line 56 at r3 (raw file):

	C c = {1};
#else
	/* This syntax casuse warning: braces around scalar initializer

casuse -> causes


utils/docker/run-build.sh, line 217 at r3 (raw file):

	printf "\n$(tput setaf 1)$(tput setab 7)BUILD ${FUNCNAME[0]} START$(tput sgr 0)\n"

	if [ ${CMAKE_VERSION_NUMBER} -lt 312 ]; then

but we never set CMAKE_VERSION_NUMBER 😉 (in pmemkv it's done in prepare-for-build.sh)

Pawel Karczewski and others added 5 commits July 19, 2021 10:15
Since C++20 classes with explicitly defaulted constructors are no more
aggregates, so constructor for initialization_list is needed.
Deprication warning combined with -Wall cause build failures for
compatibility tests
Copy link
Copy Markdown
Contributor

@igchor igchor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 5 of 11 files reviewed, 9 unresolved discussions (waiting on @KFilipek and @lukaszstolarczuk)


include/libpmemobj++/container/array.hpp, line 15 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

one block up:

#include <algorithm>
#include <functional>
#include <initializer_list>

Done.


include/libpmemobj++/container/array.hpp, line 116 at r3 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

no longer

Done.


tests/array/array_is_aggregate.cpp, line 4 at r1 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…
#include "unittest.hpp"
#include <libpmemobj++/container/array.hpp>

suparate this into 2 blocks

Done.


tests/external/libcxx/array/array.cons/implicit_copy_const.pass.cpp, line 67 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

nope..?

Done.


tests/external/libcxx/array/array.cons/initializer_list.pass.cpp, line 56 at r3 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

casuse -> causes

Done.


utils/docker/run-build.sh, line 217 at r3 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

but we never set CMAKE_VERSION_NUMBER 😉 (in pmemkv it's done in prepare-for-build.sh)

Done.

Copy link
Copy Markdown
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 5 files at r4, 2 of 2 files at r5.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @KFilipek)

@igchor igchor merged commit bad9b97 into pmem:master Jul 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants