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

nlohmann merged 14 commits into nlohmann:develop from HenryRLee:iterator_arithmetic

Jun 10, 2017

Contributor

HenryRLee commented May 27, 2017

Changes include:

Override operator+(difference_type, const iter_impl&) in the iterator class

Add const qualifier in the member operator+(difference_type) function

```
Add test cases of the relevant issue
```


          Override n + iterator operator in the iterator

ed62129

HenryRLee changed the title ~~Override operator+(n, iterator) in the iterator class~~ Issue #593 Override operator+(n, iterator) in the iterator class

coveralls commented May 27, 2017

Coverage decreased (-0.03%) to 99.686% when pulling ed62129 on HenryRLee:iterator_arithmetic into 52adf3f on nlohmann:develop.

Contributor Author

HenryRLee commented May 27, 2017

The travis test exceeded 10 mins time limit, but it seems that the other tests also took around 10 mins. Shall we extend the time limit a little?

As for the AppVeyor fail, I think it's a result of one previous PR before mine.

I really don't know what the coveralls is about. Is there a test report I can read?

HenryRLee added 4 commits

May 29, 2017 18:52


          Fix the arithmetic mistakes in reverse iterator

adc73d6


          Fix some more reverse iterator arithmetic test codes

a3bf013


          Change the definition of the operator override of reverse iterator to…

3aef1a5

… using the result of the base class directly


          Fix the indentation from the latest commit

e42db27

Contributor Author

HenryRLee commented May 29, 2017 •

edited

Loading

The latest several commits are fixing the incorrect arithmetic operations in reverse iterator.

The changes include:

Change the implementation of the arithmetic operators to using the result from the base class (std::reverse_iterator<T>) directly
Change the incorrect test cases

coveralls commented May 29, 2017

Coverage decreased (-0.03%) to 99.686% when pulling 3aef1a5 on HenryRLee:iterator_arithmetic into 52adf3f on nlohmann:develop.

coveralls commented May 29, 2017

Coverage decreased (-0.03%) to 99.686% when pulling e42db27 on HenryRLee:iterator_arithmetic into 52adf3f on nlohmann:develop.

HenryRLee added 2 commits

May 29, 2017 22:19


          Comment out the exception message check in operator[] of reverse iter…

5b245da

…ator


          Add missing test cases for n + it arithmetic in iterators

0c2ed00

Contributor Author

HenryRLee commented May 29, 2017 •

edited

Loading

The exception message checking has been commented out. Although most compilers implements the operator[] of reverse iterator using operator+, some are still using operator[] from the original iterator class. That makes the exception message unpredictable.

Contributor Author

HenryRLee commented May 29, 2017 •

edited

Loading

The other commit is adding some missing test cases for the n + iter operator.
It explicitly calls the iterator constructor during the checking, otherwise the CHECK macro doesn't understand the expression. In normal uses, the iterator constructor can be omitted.


          Use static cast in the implementation of the operators of reverse ite…

daea213

…rator

coveralls commented May 29, 2017

Coverage remained the same at 99.721% when pulling 0c2ed00 on HenryRLee:iterator_arithmetic into 52adf3f on nlohmann:develop.

HenryRLee changed the title ~~Issue #593 Override operator+(n, iterator) in the iterator class~~ Issue #593 Fix the arithmetic operators in the iterator and reverse iterator

coveralls commented May 29, 2017

Coverage decreased (-0.0003%) to 99.72% when pulling daea213 on HenryRLee:iterator_arithmetic into 52adf3f on nlohmann:develop.


          Change iterator category to random access

c4ab8f8

Owner

nlohmann commented Jun 5, 2017

@HenryRLee Could you please merge the current develop branch so the AppVeyor tests have a chance to succeed?

nlohmann requested changes

View reviewed changes

Owner

nlohmann left a comment

Hi @HenryRLee, I have some question on the changes.

test/src/unit-iterators2.cpp Outdated

                               }
                               {
                                   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");

Owner

nlohmann Jun 5, 2017

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

test/src/unit-iterators2.cpp Outdated

@@ @@ -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");

Owner

nlohmann Jun 5, 2017

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

test/src/unit-iterators2.cpp Outdated

-                                  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");
+                                  //CHECK_THROWS_WITH(it[1], "[json.exception.invalid_iterator.214] cannot get value");

Owner

nlohmann Jun 5, 2017

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

test/src/unit-iterators2.cpp Outdated

-                                  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");
+                                  //CHECK_THROWS_WITH(it[1], "[json.exception.invalid_iterator.214] cannot get value");

Owner

nlohmann Jun 5, 2017

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

test/src/unit-iterators2.cpp Outdated

-                                  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");
+                                  //CHECK_THROWS_WITH(it[1], "[json.exception.invalid_iterator.209] cannot use offsets with object iterators");

Owner

nlohmann Jun 5, 2017

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

test/src/unit-iterators2.cpp Outdated

                               }
                               {
                                   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");

Owner

nlohmann Jun 5, 2017

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

test/src/unit-iterators2.cpp Outdated

-                                  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");
+                                  //CHECK_THROWS_WITH(it[1], "[json.exception.invalid_iterator.209] cannot use offsets with object iterators");

Owner

nlohmann Jun 5, 2017

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

test/src/unit-iterators2.cpp Outdated

@@ @@ -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");

Owner

nlohmann Jun 5, 2017

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

Contributor Author

HenryRLee Jun 5, 2017

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.

Contributor Author

HenryRLee Jun 5, 2017

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.

Contributor Author

HenryRLee Jun 5, 2017

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.

Owner

nlohmann Jun 6, 2017

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?

Contributor Author

HenryRLee Jun 7, 2017

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.

test/src/unit-iterators2.cpp

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

Owner

nlohmann Jun 5, 2017

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

Contributor Author

HenryRLee Jun 5, 2017 •

edited

Loading

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

Owner

nlohmann Jun 5, 2017

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

HenryRLee added 2 commits

June 5, 2017 20:24


          Merge branch 'develop' into iterator_arithmetic

fb91aa8


          Revert "Change iterator category to random access"

c98169d

This reverts commit c4ab8f8.

coveralls commented Jun 5, 2017

Coverage decreased (-0.0003%) to 99.721% when pulling c98169d on HenryRLee:iterator_arithmetic into 1a9d766 on nlohmann:develop.

HenryRLee added 2 commits

June 7, 2017 18:01


          Revert a change in reverse iterator to pass the test cases

0f065ed


          Merge branch 'develop' into iterator_arithmetic

6661ec7

coveralls commented Jun 7, 2017

Coverage decreased (-0.0003%) to 99.72% when pulling 6661ec7 on HenryRLee:iterator_arithmetic into 92ef196 on nlohmann:develop.


          Merge branch 'develop' into iterator_arithmetic

e12c2ee

coveralls commented Jun 9, 2017

Coverage decreased (-0.0003%) to 99.722% when pulling e12c2ee on HenryRLee:iterator_arithmetic into 52f934c on nlohmann:develop.

Contributor Author

HenryRLee commented Jun 9, 2017 •

edited

Loading

Hi Niels, I think this PR is ready for review. This one is supposed to be bug fixing and adding missing operators. As for the improvement at the end of #593, I can create another PR some other time if that's needed.

nlohmann approved these changes

View reviewed changes

nlohmann self-assigned this

nlohmann added this to the Release 3.0.0 milestone

nlohmann merged commit 6caad48 into nlohmann:develop

Owner

nlohmann commented Jun 10, 2017

Thanks a lot. We shall continue the discussion in #593.

nlohmann added a commit that referenced this pull request


          ✏️ cleanup after #595

5fa5c17

Contributor Author

HenryRLee commented Jun 10, 2017

Thank you as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet