4

Looks a very straight forward case, a typical use of std::find

for ( auto element : generic->vec() )
        LOG << element;

    LOG << channel;

    if ( !gen->vec().empty() ) {

        if(std::find(generic->vec().begin(), generic->vec().end(), channel) != generic->vec().end()){

            LOG << "Found";
            ;// Found the item
        } else {

            LOG << "Not Found";
            return false;

        }
}

Please check the log file

2018-11-08, 09:37:18 [INFO] - [140455150589696] - 1
2018-11-08, 09:37:18 [INFO] - [140455150589696] - 2
2018-11-08, 09:37:18 [INFO] - [140455150589696] - 4
2018-11-08, 09:37:18 [INFO] - [140455150589696] - 12
2018-11-08, 09:37:18 [INFO] - [140455150589696] - 40
2018-11-08, 09:37:18 [INFO] - [140455150589696] - Found

The vector contains 1,2,4,12 and the incoming value that we want to test if it belongs to the vector is 40. The std::find returns true, that it is found.

The vec() method returns an array of uint64_t elements:

std::vector<uint64_t>  vec() const {
  return vec_;
}

When I am creating a local vector, ie

auto tmp = generic->vec(),

the code works.

Where is the bug in my code? I would expect to get "Not found" when checking if 40 belongs to [1,2,4,12].

Captain Nemo
  • 345
  • 2
  • 14

2 Answers2

12

The problem is that your vec function returns the vector by value. That means each call to vec will return a different and distinct vector object. And iterators from different vectors can't be compared to each other.

The simple solution is to return the vector by reference:

std::vector<uint64_t> const&  vec() const { ... }
Some programmer dude
  • 400,186
  • 35
  • 402
  • 621
3

The signature of std::vector<uint64_t> vec() const implies that an rvalue is returned (a copy of vec_). This is fine as long as you don't compare references to elements of vec() from distinct invocations. Example:

auto v1 = vec();
auto v2 = vec();

v1.begin() != v2.begin(); // Don't do that - UB

This is the same as

vec().begin() != vec().begin(); // Again, UB

Thanks to @Aconcagua for pointing out that the comparison is undefined behavior in the first place, see also this question.

Such things can be hard to spot when you're used to range based for loops, e.g.

for (const auto& value : vec()) { /* ... */ }

That's not a problem, but under the hood, this snippet is expanded such that the return value of vec() is bound to a auto&& variable and any calls to *begin/*end methods refer to the same object.

Long story short: bind the return value to a variable before requesting iterators or change the signature ov vec() such that it returns a reference, not a copy.

lubgr
  • 37,368
  • 3
  • 66
  • 117