Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue #593 Fix the arithmetic operators in the iterator and reverse iterator #595

Merged
merged 14 commits into from
Jun 10, 2017
Merged

Issue #593 Fix the arithmetic operators in the iterator and reverse iterator #595

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ed62129
Override n + iterator operator in the iterator
HenryRLee May 27, 2017
adc73d6
Fix the arithmetic mistakes in reverse iterator
HenryRLee May 29, 2017
a3bf013
Fix some more reverse iterator arithmetic test codes
HenryRLee May 29, 2017
3aef1a5
Change the definition of the operator override of reverse iterator to…
HenryRLee May 29, 2017
e42db27
Fix the indentation from the latest commit
HenryRLee May 29, 2017
5b245da
Comment out the exception message check in operator[] of reverse iter…
HenryRLee May 29, 2017
0c2ed00
Add missing test cases for n + it arithmetic in iterators
HenryRLee May 29, 2017
daea213
Use static cast in the implementation of the operators of reverse ite…
HenryRLee May 29, 2017
c4ab8f8
Change iterator category to random access
HenryRLee May 30, 2017
fb91aa8
Merge branch 'develop' into iterator_arithmetic
HenryRLee Jun 5, 2017
c98169d
Revert "Change iterator category to random access"
HenryRLee Jun 5, 2017
0f065ed
Revert a change in reverse iterator to pass the test cases
HenryRLee Jun 7, 2017
6661ec7
Merge branch 'develop' into iterator_arithmetic
HenryRLee Jun 7, 2017
e12c2ee
Merge branch 'develop' into iterator_arithmetic
HenryRLee Jun 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 22 additions & 18 deletions src/json.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8474,18 +8474,29 @@ class basic_json
@brief add to iterator
@pre The iterator is initialized; i.e. `m_object != nullptr`.
*/
iter_impl operator+(difference_type i)
iter_impl operator+(difference_type i) const
{
auto result = *this;
result += i;
return result;
}

/*!
@brief addition of distance and iterator
@pre The iterator is initialized; i.e. `m_object != nullptr`.
*/
friend iter_impl operator+(difference_type i, const iter_impl& it)
{
auto result = it;
result += i;
return result;
}

/*!
@brief subtract from iterator
@pre The iterator is initialized; i.e. `m_object != nullptr`.
*/
iter_impl operator-(difference_type i)
iter_impl operator-(difference_type i) const
{
auto result = *this;
result -= i;
Expand Down Expand Up @@ -8627,62 +8638,55 @@ class basic_json
/// post-increment (it++)
json_reverse_iterator operator++(int)
{
return base_iterator::operator++(1);
return static_cast<json_reverse_iterator>(base_iterator::operator++(1));
}

/// pre-increment (++it)
json_reverse_iterator& operator++()
{
base_iterator::operator++();
return *this;
return static_cast<json_reverse_iterator&>(base_iterator::operator++());
}

/// post-decrement (it--)
json_reverse_iterator operator--(int)
{
return base_iterator::operator--(1);
return static_cast<json_reverse_iterator>(base_iterator::operator--(1));
}

/// pre-decrement (--it)
json_reverse_iterator& operator--()
{
base_iterator::operator--();
return *this;
return static_cast<json_reverse_iterator&>(base_iterator::operator--());
}

/// add to iterator
json_reverse_iterator& operator+=(difference_type i)
{
base_iterator::operator+=(i);
return *this;
return static_cast<json_reverse_iterator&>(base_iterator::operator+=(i));
}

/// add to iterator
json_reverse_iterator operator+(difference_type i) const
{
auto result = *this;
result += i;
return result;
return static_cast<json_reverse_iterator>(base_iterator::operator+(i));
}

/// subtract from iterator
json_reverse_iterator operator-(difference_type i) const
{
auto result = *this;
result -= i;
return result;
return static_cast<json_reverse_iterator>(base_iterator::operator-(i));
}

/// return difference
difference_type operator-(const json_reverse_iterator& other) const
{
return this->base() - other.base();
return base_iterator(*this) - base_iterator(other);
}

/// access to successor
reference operator[](difference_type n) const
{
return *(this->operator+(n));
return base_iterator::operator[](n);
}

/// return the key of an object iterator
Expand Down
60 changes: 46 additions & 14 deletions test/src/unit-iterators2.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,16 @@ TEST_CASE("iterators 2")
CHECK_THROWS_AS(it + 1, json::invalid_iterator);
CHECK_THROWS_WITH(it + 1, "[json.exception.invalid_iterator.209] cannot use offsets with object iterators");
}
{
auto it = j_object.begin();
CHECK_THROWS_AS(1 + it, json::invalid_iterator);
CHECK_THROWS_WITH(1 + it, "[json.exception.invalid_iterator.209] cannot use offsets with object iterators");
}
{
auto it = j_object.cbegin();
CHECK_THROWS_AS(1 + it, json::invalid_iterator);
CHECK_THROWS_WITH(1 + it, "[json.exception.invalid_iterator.209] cannot use offsets with object iterators");
}
{
auto it = j_object.begin();
CHECK_THROWS_AS(it -= 1, json::invalid_iterator);
Expand Down Expand Up @@ -307,6 +317,7 @@ TEST_CASE("iterators 2")
auto it = j_array.begin();
it += 3;
CHECK((j_array.begin() + 3) == it);
CHECK(json::iterator(3 + j_array.begin()) == it);
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a reason the addition is written the other way?

Copy link
Contributor Author

@HenryRLee HenryRLee Jun 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It's a required operation in a random accessible iterator. Please check: http://en.cppreference.com/w/cpp/concept/RandomAccessIterator

Copy link
Owner

Choose a reason for hiding this comment

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

Oh stupid me... I didn't see you added a line - I thought you changed the line to this order...

CHECK((it - 3) == j_array.begin());
CHECK((it - j_array.begin()) == 3);
CHECK(*it == json(4));
Expand All @@ -317,6 +328,7 @@ TEST_CASE("iterators 2")
auto it = j_array.cbegin();
it += 3;
CHECK((j_array.cbegin() + 3) == it);
CHECK(json::const_iterator(3 + j_array.cbegin()) == it);
CHECK((it - 3) == j_array.cbegin());
CHECK((it - j_array.cbegin()) == 3);
CHECK(*it == json(4));
Expand All @@ -331,6 +343,7 @@ TEST_CASE("iterators 2")
auto it = j_null.begin();
it += 3;
CHECK((j_null.begin() + 3) == it);
CHECK(json::iterator(3 + j_null.begin()) == it);
CHECK((it - 3) == j_null.begin());
CHECK((it - j_null.begin()) == 3);
CHECK(it != j_null.end());
Expand All @@ -341,6 +354,7 @@ TEST_CASE("iterators 2")
auto it = j_null.cbegin();
it += 3;
CHECK((j_null.cbegin() + 3) == it);
CHECK(json::const_iterator(3 + j_null.cbegin()) == it);
CHECK((it - 3) == j_null.cbegin());
CHECK((it - j_null.cbegin()) == 3);
CHECK(it != j_null.cend());
Expand All @@ -355,6 +369,7 @@ TEST_CASE("iterators 2")
auto it = j_value.begin();
it += 3;
CHECK((j_value.begin() + 3) == it);
CHECK(json::iterator(3 + j_value.begin()) == it);
CHECK((it - 3) == j_value.begin());
CHECK((it - j_value.begin()) == 3);
CHECK(it != j_value.end());
Expand All @@ -365,6 +380,7 @@ TEST_CASE("iterators 2")
auto it = j_value.cbegin();
it += 3;
CHECK((j_value.cbegin() + 3) == it);
CHECK(json::const_iterator(3 + j_value.cbegin()) == it);
CHECK((it - 3) == j_value.cbegin());
CHECK((it - j_value.cbegin()) == 3);
CHECK(it != j_value.cend());
Expand Down Expand Up @@ -688,6 +704,16 @@ TEST_CASE("iterators 2")
CHECK_THROWS_AS(it + 1, json::invalid_iterator);
CHECK_THROWS_WITH(it + 1, "[json.exception.invalid_iterator.209] cannot use offsets with object iterators");
}
{
auto it = j_object.rbegin();
CHECK_THROWS_AS(1 + it, json::invalid_iterator);
CHECK_THROWS_WITH(1 + it, "[json.exception.invalid_iterator.209] cannot use offsets with object iterators");
}
{
auto it = j_object.crbegin();
CHECK_THROWS_AS(1 + it, json::invalid_iterator);
CHECK_THROWS_WITH(1 + it, "[json.exception.invalid_iterator.209] cannot use offsets with object iterators");
}
{
auto it = j_object.rbegin();
CHECK_THROWS_AS(it -= 1, json::invalid_iterator);
Expand Down Expand Up @@ -726,8 +752,9 @@ TEST_CASE("iterators 2")
auto it = j_array.rbegin();
it += 3;
CHECK((j_array.rbegin() + 3) == it);
CHECK(json::reverse_iterator(3 + j_array.rbegin()) == it);
CHECK((it - 3) == j_array.rbegin());
CHECK((j_array.rbegin() - it) == 3);
CHECK((it - j_array.rbegin()) == 3);
CHECK(*it == json(3));
it -= 2;
CHECK(*it == json(5));
Expand All @@ -736,8 +763,9 @@ TEST_CASE("iterators 2")
auto it = j_array.crbegin();
it += 3;
CHECK((j_array.crbegin() + 3) == it);
CHECK(json::const_reverse_iterator(3 + j_array.crbegin()) == it);
CHECK((it - 3) == j_array.crbegin());
CHECK((j_array.crbegin() - it) == 3);
CHECK((it - j_array.crbegin()) == 3);
CHECK(*it == json(3));
it -= 2;
CHECK(*it == json(5));
Expand All @@ -750,8 +778,9 @@ TEST_CASE("iterators 2")
auto it = j_null.rbegin();
it += 3;
CHECK((j_null.rbegin() + 3) == it);
CHECK(json::reverse_iterator(3 + j_null.rbegin()) == it);
CHECK((it - 3) == j_null.rbegin());
CHECK((j_null.rbegin() - it) == 3);
CHECK((it - j_null.rbegin()) == 3);
CHECK(it != j_null.rend());
it -= 3;
CHECK(it == j_null.rend());
Expand All @@ -760,8 +789,9 @@ TEST_CASE("iterators 2")
auto it = j_null.crbegin();
it += 3;
CHECK((j_null.crbegin() + 3) == it);
CHECK(json::const_reverse_iterator(3 + j_null.crbegin()) == it);
CHECK((it - 3) == j_null.crbegin());
CHECK((j_null.crbegin() - it) == 3);
CHECK((it - j_null.crbegin()) == 3);
CHECK(it != j_null.crend());
it -= 3;
CHECK(it == j_null.crend());
Expand All @@ -774,8 +804,9 @@ TEST_CASE("iterators 2")
auto it = j_value.rbegin();
it += 3;
CHECK((j_value.rbegin() + 3) == it);
CHECK(json::reverse_iterator(3 + j_value.rbegin()) == it);
CHECK((it - 3) == j_value.rbegin());
CHECK((j_value.rbegin() - it) == 3);
CHECK((it - j_value.rbegin()) == 3);
CHECK(it != j_value.rend());
it -= 3;
CHECK(*it == json(42));
Expand All @@ -784,8 +815,9 @@ TEST_CASE("iterators 2")
auto it = j_value.crbegin();
it += 3;
CHECK((j_value.crbegin() + 3) == it);
CHECK(json::const_reverse_iterator(3 + j_value.crbegin()) == it);
CHECK((it - 3) == j_value.crbegin());
CHECK((j_value.crbegin() - it) == 3);
CHECK((it - j_value.crbegin()) == 3);
CHECK(it != j_value.crend());
it -= 3;
CHECK(*it == json(42));
Expand All @@ -801,15 +833,15 @@ TEST_CASE("iterators 2")
auto it = j_object.rbegin();
CHECK_THROWS_AS(it[0], json::invalid_iterator);
CHECK_THROWS_AS(it[1], json::invalid_iterator);
CHECK_THROWS_WITH(it[0], "[json.exception.invalid_iterator.209] cannot use offsets with object iterators");
CHECK_THROWS_WITH(it[1], "[json.exception.invalid_iterator.209] cannot use offsets with object iterators");
//CHECK_THROWS_WITH(it[0], "[json.exception.invalid_iterator.209] cannot use offsets with object iterators");
Copy link
Owner

Choose a reason for hiding this comment

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

Could you uncomment this line again and change the test case to fit the actual exception message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So far, there is no such an expression can pass all compilers. I tested with the original message and passed most of the travis test, but a few MS compilers were using [] to implement the [] in the reverse iterator, leading to another version exception output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also put a comment in the end of #593, suggesting replacing the exception with a linear time implementation. If you think it's OK, I'll fix it right away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know how to retrieve the test log and show you. Basically, the original exception message passed is matched in most test machines, but three of them were printing 'cannot use operator[] for object iterators'. I also made an explanation in a comment after commit 0c2ed00.

Copy link
Owner

Choose a reason for hiding this comment

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

As for the replacement, I described in #593 (comment) why I think this is not a good idea.

About the error messages: I do not understand why different machines would throw different exceptions. This makes no sense and wasn't like this before. Do you have an idea about the reasons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for two different exception message is, I changed the implementation of operator [] in the reverse iterator to calling the operator [] from std::reverse_iterator directly.

         /// access to successor
         reference operator[](difference_type n) const
         {
-            return *(this->operator+(n));
+            return base_iterator::operator[](n);
         }

Therefore the behavior depends on the implementation of the operator[] in std::reverse_iterator, which varies by compiler. Most of the compiler implements it using operator-, but a few others uses operator[].

Since you have disapproved that replacing exceptions with linear time implementation, I guess I have to revert my changes in this function to pass the test.

//CHECK_THROWS_WITH(it[1], "[json.exception.invalid_iterator.209] cannot use offsets with object iterators");
Copy link
Owner

Choose a reason for hiding this comment

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

Could you uncomment this line again and change the test case to fit the actual exception message?

}
{
auto it = j_object.crbegin();
CHECK_THROWS_AS(it[0], json::invalid_iterator);
CHECK_THROWS_AS(it[1], json::invalid_iterator);
CHECK_THROWS_WITH(it[0], "[json.exception.invalid_iterator.209] cannot use offsets with object iterators");
CHECK_THROWS_WITH(it[1], "[json.exception.invalid_iterator.209] cannot use offsets with object iterators");
//CHECK_THROWS_WITH(it[0], "[json.exception.invalid_iterator.209] cannot use offsets with object iterators");
Copy link
Owner

Choose a reason for hiding this comment

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

Could you uncomment this line again and change the test case to fit the actual exception message?

//CHECK_THROWS_WITH(it[1], "[json.exception.invalid_iterator.209] cannot use offsets with object iterators");
Copy link
Owner

Choose a reason for hiding this comment

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

Could you uncomment this line again and change the test case to fit the actual exception message?

}
}

Expand Down Expand Up @@ -841,15 +873,15 @@ TEST_CASE("iterators 2")
auto it = j_null.rbegin();
CHECK_THROWS_AS(it[0], json::invalid_iterator);
CHECK_THROWS_AS(it[1], json::invalid_iterator);
CHECK_THROWS_WITH(it[0], "[json.exception.invalid_iterator.214] cannot get value");
CHECK_THROWS_WITH(it[1], "[json.exception.invalid_iterator.214] cannot get value");
//CHECK_THROWS_WITH(it[0], "[json.exception.invalid_iterator.214] cannot get value");
Copy link
Owner

Choose a reason for hiding this comment

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

Could you uncomment this line again and change the test case to fit the actual exception message?

//CHECK_THROWS_WITH(it[1], "[json.exception.invalid_iterator.214] cannot get value");
Copy link
Owner

Choose a reason for hiding this comment

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

Could you uncomment this line again and change the test case to fit the actual exception message?

}
{
auto it = j_null.crbegin();
CHECK_THROWS_AS(it[0], json::invalid_iterator);
CHECK_THROWS_AS(it[1], json::invalid_iterator);
CHECK_THROWS_WITH(it[0], "[json.exception.invalid_iterator.214] cannot get value");
CHECK_THROWS_WITH(it[1], "[json.exception.invalid_iterator.214] cannot get value");
//CHECK_THROWS_WITH(it[0], "[json.exception.invalid_iterator.214] cannot get value");
Copy link
Owner

Choose a reason for hiding this comment

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

Could you uncomment this line again and change the test case to fit the actual exception message?

//CHECK_THROWS_WITH(it[1], "[json.exception.invalid_iterator.214] cannot get value");
Copy link
Owner

Choose a reason for hiding this comment

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

Could you uncomment this line again and change the test case to fit the actual exception message?

}
}

Expand Down