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

string_view: implement access API and port libcxx tests#938

Merged
igchor merged 3 commits intopmem:masterfrom
KFilipek:tests-access_api
Oct 19, 2020
Merged

string_view: implement access API and port libcxx tests#938
igchor merged 3 commits intopmem:masterfrom
KFilipek:tests-access_api

Conversation

@KFilipek
Copy link
Copy Markdown
Contributor

@KFilipek KFilipek commented Oct 16, 2020

  • string_view: add string_view access API libcxx tests
  • string_view: implement access API
  • string_view: port LIBCXX access tests to use pmem::obj::string_view

This change is Reviewable

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.

:lgtm:

Reviewed 7 of 7 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 16, 2020

Codecov Report

Merging #938 into master will decrease coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #938      +/-   ##
==========================================
- Coverage   96.07%   95.98%   -0.10%     
==========================================
  Files          48       48              
  Lines        6122     6134      +12     
==========================================
+ Hits         5882     5888       +6     
- Misses        240      246       +6     
Flag Coverage Δ
#tests_clang_debug_cpp17 95.94% <ø> (-0.09%) ⬇️
#tests_gcc_debug 92.37% <100.00%> (-0.19%) ⬇️

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

Impacted Files Coverage Δ
include/libpmemobj++/string_view.hpp 100.00% <100.00%> (ø)
include/libpmemobj++/condition_variable.hpp 73.80% <0.00%> (-4.77%) ⬇️
...j++/container/detail/concurrent_skip_list_impl.hpp 97.99% <0.00%> (-0.27%) ⬇️
...ude/libpmemobj++/container/concurrent_hash_map.hpp 93.76% <0.00%> (-0.21%) ⬇️

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 3a9343d...4e17da1. Read the comment docs.

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 6 of 7 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @KFilipek)


include/libpmemobj++/string_view.hpp, line 166 at r2 (raw file):

 * Returns count of characters stored in this pmem::obj::string_view data.
 *
 * @return pointer to C-like string (char *), it may not end with null

returns pointer to c-like string? (here and above)


include/libpmemobj++/string_view.hpp, line 206 at r2 (raw file):

/**
 * Returns reference to a last character in the view.

to the last char


include/libpmemobj++/string_view.hpp, line 219 at r2 (raw file):

/**
 * Returns reference to a first character in the view.

.


include/libpmemobj++/string_view.hpp, line 220 at r2 (raw file):

/**
 * Returns reference to a first character in the view.
 *

perhaps also worth mention what if empty() == true


tests/external/libcxx/string.view/string.view.access/at.pass.cpp, line 37 at r2 (raw file):

	}

#ifndef TEST_HAS_NO_EXCEPTIONS

I know it's ported from libcxx, but I'd remove this if. We throw exceptions in at and if we ever change it, this test will just fail... which is good, I guess


tests/external/libcxx/string.view/string.view.access/back.pass.cpp, line 55 at r2 (raw file):

	{
		constexpr pmem::obj::basic_string_view<char> sv("ABC", 2);
		static_assert(sv.length() == 2, "");

why not UT_ASSERT?


tests/external/libcxx/string.view/string.view.access/data.pass.cpp, line 49 at r2 (raw file):

		constexpr const char *s = "ABC";
		constexpr pmem::obj::basic_string_view<char> sv(s, 2);
		static_assert(sv.length() == 2, "");

.


tests/external/libcxx/string.view/string.view.access/front.pass.cpp, line 54 at r2 (raw file):

	{
		constexpr pmem::obj::basic_string_view<char> sv("ABC", 2);
		static_assert(sv.length() == 2, "");

.


tests/external/libcxx/string.view/string.view.access/index.pass.cpp, line 59 at r2 (raw file):

		constexpr pmem::obj::basic_string_view<char> sv("ABC", 2);
		static_assert(sv.length() == 2, "");
		static_assert(sv[0] == 'A', "");

.

Copy link
Copy Markdown
Contributor Author

@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 @lukaszstolarczuk)


include/libpmemobj++/string_view.hpp, line 166 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

returns pointer to c-like string? (here and above)

No, I don't see any reason to duplicate "return" word:
https://pmem.io/libpmemobj-cpp/master/doxygen/classpmem_1_1obj_1_1basic__string.html#afec7256ef676461b095a0e2f662893e0
The same here in return section:
https://en.cppreference.com/w/cpp/string/basic_string_view/front


include/libpmemobj++/string_view.hpp, line 206 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

to the last char

Done.


include/libpmemobj++/string_view.hpp, line 219 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

.

Done.


include/libpmemobj++/string_view.hpp, line 220 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

perhaps also worth mention what if empty() == true

Done.


tests/external/libcxx/string.view/string.view.access/at.pass.cpp, line 37 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

I know it's ported from libcxx, but I'd remove this if. We throw exceptions in at and if we ever change it, this test will just fail... which is good, I guess

Done.


tests/external/libcxx/string.view/string.view.access/back.pass.cpp, line 55 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

why not UT_ASSERT?

because static_assert is evaluated in compile-time in opposite to assert which is underlying.


tests/external/libcxx/string.view/string.view.access/data.pass.cpp, line 49 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

.

The same here, above is the constexpr


tests/external/libcxx/string.view/string.view.access/front.pass.cpp, line 54 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

.

.


tests/external/libcxx/string.view/string.view.access/index.pass.cpp, line 59 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

.

.

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.

Reviewed 1 of 2 files at r3.
Reviewable status: 6 of 7 files reviewed, 10 unresolved discussions (waiting on @KFilipek and @lukaszstolarczuk)


include/libpmemobj++/string_view.hpp, line 13 at r1 (raw file):

#include <algorithm>
#include <stdexcept>

You should leave stdexcept here - for out_of_range

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 2 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @KFilipek)


include/libpmemobj++/string_view.hpp, line 166 at r2 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

No, I don't see any reason to duplicate "return" word:
https://pmem.io/libpmemobj-cpp/master/doxygen/classpmem_1_1obj_1_1basic__string.html#afec7256ef676461b095a0e2f662893e0
The same here in return section:
https://en.cppreference.com/w/cpp/string/basic_string_view/front

no, no. You didn't understand me ;) this are length and size - do they return a pointer to a string? I don't thinks so... ;)

- add missing functions: front, back
- extend declarations by constexpr keyword
- change implementation to use pmem::obj::string_view
Copy link
Copy Markdown
Contributor Author

@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: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @igchor and @lukaszstolarczuk)


include/libpmemobj++/string_view.hpp, line 13 at r1 (raw file):

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

You should leave stdexcept here - for out_of_range

Done.


include/libpmemobj++/string_view.hpp, line 166 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

no, no. You didn't understand me ;) this are length and size - do they return a pointer to a string? I don't thinks so... ;)

Done. Sorry xD

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 1 of 1 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @igchor)


include/libpmemobj++/string_view.hpp, line 166 at r2 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

Done. Sorry xD

no problem, thx

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.

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@igchor igchor merged commit ca9562b into pmem:master Oct 19, 2020
@KFilipek KFilipek deleted the tests-access_api branch November 25, 2020 10:40
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.

3 participants