2

I am trying to reverse a std::string:

void reverseString(vector<char>& s)
{
   auto i = s.begin();
   auto j = s.rbegin();

   while (i != j) 
   {
      char tmp = *i;
      *i = *j;
      *j = tmp;

      i++;
      j--;
   }
}

However, this happened when I tried to compare iterators

ERROR:

Line 6: Char 17: error: invalid operands to binary expression 
('__gnu_cxx::__normal_iterator<char *, std::vector<char, std::allocator<char> > >' and
'std::reverse_iterator<__gnu_cxx::__normal_iterator<char *, std::vector<char, std::allocator<char> > > >')
        while(i != j) {

JeJo
  • 30,635
  • 6
  • 49
  • 88
HOODINI
  • 53
  • 1
  • 5
  • 3
    begin() and rbegin() return different types. One is an iterator another one is a reverse iterator. – dgrandm Jul 21 '20 at 05:31
  • @dgrandm Picked that up for my answer. I hope you do not mind. – Yunnosch Jul 21 '20 at 05:35
  • Please see this https://gcc.godbolt.org/z/r1vv73 and compare. –  Jul 21 '20 at 05:56
  • @VarushVarsha That is link-only, code-only and answer-in-a-comment. Ok, maybe you did comment instead of answering BECAUSE it is link-only and code-only. Please consider making a proper, explained answer directly here. If with an explanation I can easily see that it is better than my answer I will happily upvote. – Yunnosch Jul 21 '20 at 05:59
  • I haven't down voted your answer; as far as a proper answer is concerned, neither I have the time nor I have the willingness to get properly involved with this website. –  Jul 21 '20 at 06:05
  • It is not about the downvote. I want to harvest your helpful contribution. Please consider giving a short explanation in a comment and allowing others to use it for making a complete answer. That way we get a win-win-win (OP, other author, the community). Surely you would get some credits in an answer based on your comment, making even for a fourth "win". ;-) Have fun. @VarushVarsha – Yunnosch Jul 21 '20 at 06:09

3 Answers3

3

You have a few issues in your code:

  • Firstly i has the type std::vector<char>::iterator and the j has the type std::vector<char>::reverse_iterator, which are not same. Therefore you can not do
    while(i != j) 
    
    That is the reason for the compiler error!
  • Secondly, a reverse iterator should be as like a normal iterator. enter image description here Meaning j--; will try to move one past which is an end iterator and dereferencing in in next iteration that invokes undefined behaviour. You should be instead, incrementing it to iterate from the last of the container.
  • Last but not least, you should only go to half of the container to reverse it. Otherwise, you will be swapping twice and will get the same vector you passed.

Following is the corrected code: See a demo

#include <iostream>
#include <iterator>
#include <algorithm> // std::copy_n, std::swap
#include <vector>

void reverseString(std::vector<char>& s)
{
   auto i = s.begin();
   auto j = s.rbegin();
   const auto half = s.size() / 2u;

   while (i != s.begin() + half || j != s.rbegin() + half)
   {
      // your saping code or simply `std::swap` the elements
      std::swap(*i, *j);
      ++i;
      ++j; // increment both iterators
   }
}

int main()
{
   std::vector<char> vec{ 'a', 'b', 'c' };
   reverseString(vec);
   // print the reversed vector
   std::copy_n(vec.cbegin(), vec.size(), std::ostream_iterator<char>(std::cout, " "));
}

Output:

c b a 

As a side note:

JeJo
  • 30,635
  • 6
  • 49
  • 88
  • Fair enough. You do have an unusual and very appreciated amount of explanation. I like it as kind of a practically oriented sibling to my more philosophical answer. ;-) – Yunnosch Jul 21 '20 at 06:48
  • Let's see about "last". I would not be surprised by more different angles being presented. – Yunnosch Jul 21 '20 at 06:50
1

The error message already says it

('__gnu_cxx::__normal_iterator<char *, std::vector<char, std::allocator > >' and 'std::reverse_iterator<__gnu_cxx::__normal_iterator<char *, std::vector<char, std::allocator > > >')

When you remove the common, irrelevant stuff inside the angle brackets, you get

__gnu_cxx::__normal_iterator<...> and std::reverse_iterator<__gnu_cxx::__normal_iterator<...> >

This means you have two different types, for which no appropriate comparison operator is defined.


To solve this, you might compare with std::reverse_iterator::base(), e.g.

while (i != j.base()) {
    // ...
}

But also note

The base iterator refers to the element that is next (...) to the element the reverse_iterator is currently pointing to.

This means, you must also check one beyond and the loop condition becomes

while (i != j.base() && i != j.base() - 1) {
    // ...
}

Unrelated, but instead of doing the swap yourself manually, you might use std::iter_swap

while (i != j.base() && i != j.base() - 1) {
    std::iter_swap(i, j);
    ++i;
    ++j;
}
Olaf Dietsche
  • 72,253
  • 8
  • 102
  • 198
0

It seems you know that you have to dereference iterators as in *i = *j;.
You also need to do that in while(i != j), because you can compare chars, but not iterators. Especially not iterators of different kinds, as dgrandm mentions in his comment.

I.e.

while(*i != *j)

This gets the type problem fixed.
It is however likely that you are actually trying to not compare the value of the elements but instead their position in the vector. I.e. in order to reverse the whole vector.
Comparing positions is not possible with iterators
(to be precise: comparing for lesser/greater, see below on the appreciated input by Alan Birtles). Because iterators intentionally hide the differences between different containers. Vectors do have a comparable position, but other containers do not have one. Think of linked lists for example.
You would not be able to compare positions in a linked list, at least not by comparing iterators. Hence the container-abstracting iterators also hide the comparability. This in turn means that you cannot compare positions via iterators, even if the container you are currently using would allow it.

To clarify: Comparing iterators is possible (like "is this the same element?"), as Alan Birtles mentions, but only for equality. The comparision you need however is for lesser/greater (like "did these two iterators pass each other?"). This becomes apparent if you think of containes with an even number of elements. In that case with i++;j--; the two iterators never point to the same element and your loop would fail. You could equality-check before AND after changing the second iterator, if that were possible for the two DIFFERENT iterators you are using.

Yunnosch
  • 26,130
  • 9
  • 42
  • 54
  • I would be interested in the reason for the downvote. Please note that my answer is different from the comment I used and that I only added it as a detail when I found it after finishing editing and posting. If the downvote is by the comment author, let me know. I will remove the comment detail. – Yunnosch Jul 21 '20 at 05:38
  • It's probably due to the incorrectness. Comparing iterators runs the loop until the _positions_ are equal. Comparing values runs the loop until the _elements_ are equal, which is not useful for reversing. – chris Jul 21 '20 at 05:42
  • 1
    @SaiSreenivas Good point. You are saying that there is more to fix than the type problem. I will incorporate that. – Yunnosch Jul 21 '20 at 05:42
  • @chris Same as above. Thanks. – Yunnosch Jul 21 '20 at 05:42
  • I don't think `Comparing positions is not possible with iterators` is correct, you can't compare iterators of different types but iterators from the same container and of the same type definitely are comparable – Alan Birtles Jul 21 '20 at 06:22
  • @AlanBirtles You probably mean comparing for equality and you are right. For the goal (at least for even numbers of elements), comparing for lesser/greater is probably necessary. I will incorporate your input - somehow. Thanks. – Yunnosch Jul 21 '20 at 06:25
  • @AlanBirtles I think I got it incorporated. Very good input anyway. Thanks. – Yunnosch Jul 21 '20 at 06:34