Skip to content

Conversation

@sdkrystian
Copy link
Member

No description provided.

@sdkrystian
Copy link
Member Author

Tests are failing as these changes are applied separately from #54, will pass once it is applied.

@vinniefalco
Copy link
Member

If one commit requires the other then they should be part of the same pull request. One commit per bugfix doesn't mean you have to put each one into its own pull request.

// move semantics.
if (what_ == what::strfunc)
return *static_cast<const string*>(f_.p);
else
Copy link
Member

Choose a reason for hiding this comment

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

I would lose the else, it reads better

// nullptr, and capacity returns 0
// when the table pointer is null
array b{std::move(a), object()};
BOOST_TEST(a.capacity() == 0);
Copy link
Member

Choose a reason for hiding this comment

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

I would remove these comments because they don't provide any additional information

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

if(e.what_ != what::str &&
e.what_ != what::strfunc)
return false;
return true;
Copy link
Member

Choose a reason for hiding this comment

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

Yep this looks much better without the else, as it is absolutely clear that the function ends on line 52

@sdkrystian sdkrystian changed the title Add moving of strings from value_ref Add moving of strings from value_ref, fix value constructor Mar 8, 2020
@codecov
Copy link

codecov bot commented Mar 8, 2020

Codecov Report

Merging #55 into develop will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop      #55   +/-   ##
========================================
  Coverage    99.09%   99.09%           
========================================
  Files           57       57           
  Lines         4854     4861    +7     
========================================
+ Hits          4810     4817    +7     
  Misses          44       44           
Impacted Files Coverage Δ
include/boost/json/impl/value_ref.ipp 97.26% <100.00%> (+0.24%) ⬆️
include/boost/json/value.hpp 100.00% <100.00%> (ø)
include/boost/json/value_ref.hpp 100.00% <100.00%> (ø)

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 37d0a09...fdc86db. Read the comment docs.

@vinniefalco
Copy link
Member

Merged, thanks!

@vinniefalco vinniefalco closed this Mar 9, 2020
@sdkrystian sdkrystian deleted the value-ref-string branch June 10, 2020 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants