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

Conversation

@KFilipek
Copy link
Contributor

@KFilipek KFilipek commented Oct 20, 2020

This PR contains:

  • string_view: add string_view capacity API libcxx tests
  • string_view: implement modifiers API for string_view
  • string_view: port modifiers libcxx tests

This change is Reviewable

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


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

 * The behavior is undefined if n > size().
 *
 * @param[in] n of characters to remove from the start of the view

number/count of characters to...


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

/**
 * Exchanges the view with that of v.
 * 

redundant white space


tests/external/libcxx/string.view/string.view.modifiers/remove_prefix.pass.cpp, line 8 at r1 (raw file):

//
//===----------------------------------------------------------------------===//

licenses :P

Copy link
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.

Reviewed 1 of 5 files at r1, 4 of 4 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lukaszstolarczuk)


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

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

number/count of characters to...

Done. Now should sound natural, I want to leave 'n' as an parameter.


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

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

redundant white space

Done.


tests/external/libcxx/string.view/string.view.modifiers/remove_prefix.pass.cpp, line 8 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

licenses :P

Done.

@codecov
Copy link

codecov bot commented Oct 22, 2020

Codecov Report

Merging #946 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #946      +/-   ##
==========================================
+ Coverage   95.93%   95.95%   +0.02%     
==========================================
  Files          48       48              
  Lines        6146     6154       +8     
==========================================
+ Hits         5896     5905       +9     
+ Misses        250      249       -1     
Flag Coverage Δ
#tests_clang_debug_cpp17 95.79% <ø> (-0.02%) ⬇️
#tests_gcc_debug 92.42% <100.00%> (+0.01%) ⬆️

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%> (ø)
...ude/libpmemobj++/container/concurrent_hash_map.hpp 93.36% <0.00%> (-0.11%) ⬇️
include/libpmemobj++/condition_variable.hpp 78.57% <0.00%> (+4.76%) ⬆️

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 55351ae...5f02ba8. Read the comment docs.

Copy link
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 5 files at r1, 4 of 4 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lukaszstolarczuk)


include/libpmemobj++/string_view.hpp, line 299 at r4 (raw file):

	basic_string_view<CharT, Traits> &v) noexcept
{
	const value_type *p = data_;

why not just:

std::swap(data_, v.data_);
std::swap(size_, v.size_);

?

Copy link
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.

Reviewed 5 of 5 files at r6.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lukaszstolarczuk)


include/libpmemobj++/string_view.hpp, line 299 at r4 (raw file):

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

why not just:

std::swap(data_, v.data_);
std::swap(size_, v.size_);

?

Hmm, I've copied libcxx function body, but std::swap is also marked as constexpr

Copy link
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 1 of 4 files at r4, 1 of 5 files at r6.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @KFilipek)


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

Previously, KFilipek (Krzysztof Filipek) wrote…

Done. Now should sound natural, I want to leave 'n' as an parameter.

it seems good, but it does not point that it is a number/count of chars to remove (one may interpret it as a charset to remove 😉 )

Copy link
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: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @KFilipek and @lukaszstolarczuk)


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

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

it seems good, but it does not point that it is a number/count of chars to remove (one may interpret it as a charset to remove 😉 )

Done. I've changed it according to pmem::obj::string documentation

Copy link
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 3 of 5 files at r6, 1 of 1 files at r7.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
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 1 of 1 files at r7.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

- add remove_prefix, remove_suffix and swap for string_view
- enable tests for remove_suffix/prefix and swap methods
Copy link
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 2 of 2 files at r8.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
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 r8.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@lukaszstolarczuk lukaszstolarczuk merged commit 928acd8 into pmem:master Oct 27, 2020
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