0

I did the following as a cheap way to allow read-only access to a member container _numbers via numbers:

class Foo {
    Foo() : _numbers({}), numbers(_numbers) {
    // some code that populates `numbers` via `numbers.push_back(...)`
}

private:
    std::vector<int> _numbers;
public:
    const std::vector<int>& numbers;
}

However, doing so I see that numbers is empty, while in other cases it will contain the same elements as _numbers.

To be more precise, it seems to be undefined behavior. In my real example (of which this a simplified version) I have multiple reference-container pairs with this scheme, where the populated data is visible in the const-reference member for some pairs, and for some it is not.

Any idea whats wrong with this? Any help is deeply appreciated.

EDIT Here is a minimal working example:

#include <vector>

struct Foo2 {
public:
     const int max1;
     const int center1;

    Foo2(const int max1_);
private:
    std::vector<int> _numbers1, _numbers2;

public:
    const std::vector<int>& numbers1, numbers2;
};

Foo2::Foo2(const int max1_)
    : max1(max1_), center1(max1_/2),
      _numbers1({}), _numbers2({}),
      numbers1(_numbers1),
      numbers2(_numbers2)
{
    cout << max1 << endl;

    for (int i=0; i<center1; i++) {
        _numbers1.push_back(i);
        cout << "A: " << i << endl;
    }
    for (int i=center1; i<max1; i++) {
        _numbers2.push_back(i);
        cout << "B: " << i << endl;
    }

    for (int i: numbers1) {
        cout << "A: " << i << endl;
    }
    for (int i: numbers2) {
        cout << "B: " << i << endl;
    }
}

which gives the following Output when initializing Foo2 f(8):

8
A: 0
A: 1
A: 2
A: 3
B: 4
B: 5
B: 6
B: 7
A: 0
A: 1
A: 2
A: 3

i.e. numbers2 does not see the contents of _numbers2 while for numbers1 it seems to work.

flonk
  • 3,726
  • 3
  • 24
  • 37
  • 1
    I can't reproduce any problems. Could you provide example code where something unexpected happens? – Stefan Riedel Jun 24 '21 at 17:21
  • It might take a while because the real code is quite complex, but I'll try... – flonk Jun 24 '21 at 17:22
  • 1
    Some problems I can think of are copying/moving. You should provide copy/move constructor and assignment operators. problems like this: https://godbolt.org/z/fWhaTzsq7 – Stefan Riedel Jun 24 '21 at 17:24
  • 1
    Note references cannot be reseated, so this makes `Foo` hard to assign. If this is an issue, I'd use a getter that returns a `const` reference. Any decent compiler will optimize that down to nothing or next-to-nothing. – user4581301 Jun 24 '21 at 17:25
  • @StefanRiedel Thanks, but I guess the problem is not copy/move, because I print out `numbers` immediately after populating `_numbers` already inside the constructor and see differences – flonk Jun 24 '21 at 17:27
  • Sounds weird, but I agree with @user4581301, you should just go the common way and provide a method returning a const reference `const std::vector& numbers() const {return _range;}`. Anyway I'm interested in the actual problem here. – Stefan Riedel Jun 24 '21 at 17:28
  • Thanks but I'd like to avoid the method-approach because it makes my code look cleaner without the calling brackets, as I need to iterate over these numbers very often. – flonk Jun 24 '21 at 17:32
  • It might take while because the problem appears to be non-deterministic, so I have to simplify my original while still seeing the problem. – flonk Jun 24 '21 at 17:34
  • Well that's a quite small "inconvenience". I would totally go for the method. And your code is probably not well designed if you have a lot of code sections where you iterate over those numbers. But that's just speculating. – Stefan Riedel Jun 24 '21 at 17:36
  • Ok. I can see the problem with the brackets everywhere. How about overloading `operator[]`? `int operator[](size_t index) { return _numbers[index]; }`? Now you can `fooinstannce[42]` and pass on exposing the `vector` at all. – user4581301 Jun 24 '21 at 17:41
  • @user4581301 actually not seeing any problem with those brackets. `for(auto i : myFoo.range()) {...}` looks totally fine. And depending on the actual class and the actual vector is an operator[] quite unusual. Like a `Car` class having a member `vector _tires`. I wouldn't want to access those tires with `myCar[3]`... – Stefan Riedel Jun 24 '21 at 17:45
  • Depends on what you're doing and how you do it. Your pitch is clean, but `myFoo.numbers()[42]` looks clunky as hell. Only marginally worse than `myFoo.numbers[42]`, mind you. `myFoo[42]` is pretty slick and the user has no clude you even have a vector in there. Go to the next stp and wrap `vector`'s iterator interface, at least the `const` one and you get `for(auto i : myFoo)`. And if down the road the `vector` is replaced with a `map` for a sparse array no one is the wiser. – user4581301 Jun 24 '21 at 17:51
  • @StefanRiedel Now that I see your comment edit, I agree `car[3]` would be stupid. I wouldn't go down either route unless `Foo` is nothing but a container class. If it has more meaning than data, I'd expose none (or as close as possible) of its internals and expose functions that operate on the `Foo` and get a result rather than getting data from the `Foo` and operating on the data outside the `Foo` to get the result. `car.inflate_tire(3)` instead of `car.tire[3].inflate()`. – user4581301 Jun 24 '21 at 17:55
  • You could also implement something like C# properties. Like [this approach](https://stackoverflow.com/questions/59338030/c-property-template-for-classes-without-standard-constructor) (there are better ones, I just didn't find them quickly). But that is a lot of bloat just to get rid of those 2 brackets... – Stefan Riedel Jun 24 '21 at 18:00
  • @StefanRiedel I've added a working minimal example now. – flonk Jun 24 '21 at 18:08
  • *"minimal working example"* Nope, it's not one. `error: 'vector' does not name a type`, etc. – HolyBlackCat Jun 24 '21 at 18:11
  • If it seems like we're being unnecessarily picky, it's because every change we make to your example is an opportunity to insert a new bug (and give useless answers based on that bug) or accidentally fix the problem (and give no answers). – user4581301 Jun 24 '21 at 18:15
  • No it's fine, I've added the `std::` prefixes ;) – flonk Jun 24 '21 at 18:15
  • Next time, please also add the headers. – HolyBlackCat Jun 24 '21 at 18:19
  • Actually it was the lack of `#include ` that really got you. I'll tolerate a `using namespace std;` in a toy example. – user4581301 Jun 24 '21 at 18:19
  • `error: 'cout' was not declared in this scope` — You try to compile your code before posting it, do you? See [mcve] for why we ask for it. – HolyBlackCat Jun 24 '21 at 18:29

2 Answers2

5

const vector<int>& numbers1, numbers2; — Here, only the first variable is a reference. You need & before the second variable to make it a reference as well. Then the code should work.

But I have to say that what you're doing is a really bad idea. You're paying for a convenient syntax with memory overhead, non-assignability, and possibly speed overhead.

Use getters instead: const vector<int>& numbers1() const {return _numbers1;}. Yes, you will have to type the extra () every time.

HolyBlackCat
  • 78,603
  • 9
  • 131
  • 207
  • 1
    *facepalm*, thanks very much, I knew this for pointers but never stepped into this trap for references. – flonk Jun 24 '21 at 18:16
  • Thanks for Your further comments, can You explain why my approach has *memory*-overhead against the getter-approach (assuming that `numbers1` is accessed at least once)? – flonk Jun 24 '21 at 18:22
  • @flonk In practice, references have the same size as pointers. So (assuming 64-bit platform) you're wasting 8 bytes per reference per `class Foo` instance. I've never seen a compiler optimizing away a class member reference. (Unlike function-local references.) – HolyBlackCat Jun 24 '21 at 18:27
  • Thanks, so what You say is that with the getter-approach doing `Foo foo(4); for (int k: foo.numbers1()) { ... }`, will be optimized in such a way that no reference is created at all, but becomes technically equivalent to the direct loop over the private vector `foo._numbers1`? – flonk Jun 24 '21 at 18:31
  • @flonk Yes, that should be the case. At least if you enable compiler optimizations, and if the method implementation isn't hidden in a `.cpp` file. – HolyBlackCat Jun 24 '21 at 18:32
  • 2
    upvoting the answer for "what you're doing is a really bad idea" – Stefan Riedel Jun 24 '21 at 18:42
0

Hmm... The main cause of the problem was given by @HolyBlackCat: numbers2 was not a reference but an independant copy.

But IMHO there is a more fundamental problem. With:

public:
    const std::vector<int>& numbers;

You promise the compiler, that once initialized, the vector referenced by numbers will no longer change. Which is a lie, because the underlying vector changes...

Lying to the compiler is a sure path to UB: it will work sometimes, and will suddenly break because the compiler is free to change its actual code so that no changes to _member will be reflected in members.

Possible fixes:

  • use std::vector<int>& numbers = _numbers; (no const). This one will be fine, but you loose the ability to present a read only reference

  • get a fully initialized reference when you need it through a getter (i.e. a method):

      const std::vector& numbers() {
          return _numbers;
     }
    

    Again it is fine, provided _number no longer changes after complete initialization of Foo

  • use a dedicated object, implicitely convertible to a vector that will be fully initialized before being used:

      struct Bar {
          std::vector<int>_numbers;
    
          Bar(bool first) : Bar(0, first) {};
          Bar(int max, bool first) {
              cout << max << endl;
              int center = max / 2;
    
              if (first) {
                  for (int i = 0; i < center; i++) {
                      _numbers.push_back(i);
                      cout << "A: " << i << endl;
                  }
              }
              else {
                  for (int i = center; i < max; i++) {
                      _numbers.push_back(i);
                      cout << "B: " << i << endl;
                  }
              }
          }
    
          operator const std::vector<int>& () {
              return _numbers;
          }
      };
      struct Foo2 {
      public:
          const int max1;
          const int center1;
    
          Foo2();
          Foo2(const int max1_);
      private:
          Bar _numbers1, _numbers2;
    
      public:
          const std::vector<int>& numbers1, &numbers2;
      };
    
Serge Ballesta
  • 143,923
  • 11
  • 122
  • 252