0

I've written a class that looks like

class Mesh {
    public:
        vector<Vertex> vs;
}

where Vertex is

class Vertex {
    public:
        const double x, y, z;
}

I have a function which loads a Mesh from a file:

shared_ptr<Mesh> load_mesh(string filename) {
    //....
    vector<Vertex> vs;
    Vertex v(1, 2, 3);
    vs.push_back(v);
    return shared_ptr<Mesh>(Mesh(vs));
}

My questions are concerning the scope of the Vertex and the vector.

Will one or both go out of scope?

Which (if any) of the alternatives are preferred?

class Mesh1 {
    public:
        vector<shared_ptr<Vertex>> vs;
}


class Mesh2 {
    public:
        shared_ptr<vector<Vertex>> vs;
}


class Mesh3 {
    public:
        shared_ptr<vector<shared_ptr<Vertex>>> vs;
}

Or is there a better / easier way of handling this?

sdasdadas
  • 23,917
  • 20
  • 63
  • 148
  • 2
    Your original should be fine. `push_back` stores a copy of `v` in the local `vs`; and given the way you declared `Mesh`, constructing one should make a copy of the local `vs` along with all of its elements. The local variables will go out of scope and be destroyed, but the copy in `Mesh` is safe. – T.C. Sep 20 '14 at 21:54
  • Ah, sorry, fixed that. Thanks for the response! – sdasdadas Sep 20 '14 at 21:57
  • 4
    Seems like you're asking about the scope of objects inside functions, not the scope of classes inside classes. – Lightness Races in Orbit Sep 20 '14 at 22:00
  • Are you sure you don't want `std::vector`? – Deduplicator Sep 20 '14 at 22:18
  • **-1** not real code. even after adding all the missing semicolons this code won't compile. that said, just forget about `new`: return whatever you want to return and let the compiler worry about optimization. adding costly dynamic allocations and indirection is not necessarily the best way to increase performance. – Cheers and hth. - Alf Sep 20 '14 at 23:37
  • I suggest you don't add const members to Vertex, and instead create a const Vertex object when necessary. – Neil Kirk Sep 21 '14 at 00:39

1 Answers1

1

Your basic structure looks right to me. You are copying the Vertex into the vector and then copying the vector into the Mesh. The local copies in the load_mesh() function will go out of scope but because you have made a copy that is ok.

At the risk of being accused of premature optimization I would say that unless the vector is small all that copying is a little inefficient. There are a number of ways it could be optimized. With C++11, and move semantics, you can keep your current structure and just move the data:

#include <vector>

struct Vertex {
  const double x, y, z;
  Vertex(double _x, double _y, double _z) : x(_x), y(_y), z(_z) {} 
};

struct Mesh {
  std::vector<Vertex> vs;
  Mesh(std::vector<Vertex> _vs) : vs(std::move(_vs)) {}
  Mesh(Mesh&& other) noexcept : vs(std::move(other.vs)) {}  // Move constructor
};

Mesh
loadMesh() {
  //....
  std::vector<Vertex> vs;
  vs.emplace_back(1,2,3);
  return Mesh{std::move(vs)};
}

int main() {
  auto mesh = loadMesh();
}

I'm using emplace_back instead of push_back to construct the Vertex in the vector in-place and using std::move to move the vector into Mesh.

Returning a shared_ptr<Mesh> would be fine but I wanted to show you can also return the Mesh by value. The compiler should perform RVO and there will be no copy (see this question).

Community
  • 1
  • 1
Chris Drew
  • 14,926
  • 3
  • 34
  • 54
  • Your Mesh-ctor misses `&&`. – Deduplicator Sep 20 '14 at 23:09
  • @Deduplicator, No [I've intentionally used pass-by-value](http://stackoverflow.com/questions/7592630/is-pass-by-value-a-reasonable-default-in-c11). At the price of only one extra move I only have to define one constructor but can still bind to rvalues or lvalues. Yes, you could restrict to rvalues if you want to. – Chris Drew Sep 20 '14 at 23:14
  • @Deduplicator `Mesh` does need a copy, it needs it in it's member variable. Maybe that is a misleading link, try [this one](http://stackoverflow.com/questions/21963062/best-way-to-write-constructor-of-a-class-who-holds-a-stl-container-in-c11): – Chris Drew Sep 20 '14 at 23:22
  • i'm not sure when or whether move constructor is generated. i would feel more sure of the code if there was an explicitly specified move constructor. – Cheers and hth. - Alf Sep 20 '14 at 23:41
  • @Cheersandhth.-Alf Yeah, but then I would have to define a normal constructor and realistically a copy constructor also. To keep it simple I might just define a normal constructor and rely on `emplace_back`... – Chris Drew Sep 21 '14 at 00:17
  • As it happens, Visual C++ 2013 (i.e. version 12.0) generates a copy constructor call for the return in your current code. Just adding `Mesh(Mesh&& other): vs( move( other.vs ) ) {}` fixes this. – Cheers and hth. - Alf Sep 21 '14 at 01:55
  • @Cheersandhth.-Alf. Oh, that's a bit disappointing. I thought you could rely on RVO. Interesting if I change it to NVRO by creating a local mesh variable it does seem to optimize it. I might create a question about it. – Chris Drew Sep 21 '14 at 07:36