4

I want to write an c++ class that wraps a c struct. Here is a simple example for a c struct:

struct POINT {
  long x;
  long y;
}

Now I am assuming this following class, but I am not sure if it is "performant" nor good c++ style. I didnt want to use a unnecessary variable or function call. It would be very nice if you improve my code :).

The basic idea behind this class is that it is just a wrapper/ handler for the struct. Thats why setStruct and getStruct can modify the private data directly and that it is just a pointer. The other members are always named set<Attribute> and get<Attribute>.

If you use setStruct the only disadvantage I can think of is that the struct could be delete due to scopes so that the pointer is "invalid".

namespace wrapper {
class POINT {
  ::POINT * POINT_;

public:
  POINT() {
    POINT_ = new ::POINT;
  }
  ~POINT() {
    delete POINT_;
  }
  inline void setX( long x ) {
    POINT_->x = x;
  }
  inline long getX() {
    return POINT_->x;
  }
  inline void setY( long y ) {
    POINT_->y = y;
  }
  inline long getY() {
    return POINT_->y;
  }
  inline void setStruct(::POINT * __POINT) {
    POINT_ = __POINT;
  }
  inline ::POINT * getStruct() {
    return POINT_;
  }
};
}
andrew
  • 97
  • 1
  • 10
  • 1
    Why not just hold 2 LONG member variables and define an implicit conversion operator to POINT ? Also theres absolutely no reason to hold a pointer to a ::POINT instead of the ::POINT itself. – Borgleader Jul 03 '13 at 17:32
  • 2
    `_POINT` is a [reserved identifier](http://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier). Also, never use `new` except maybe in `std::make_unique` until C++14 or unless you're the 1% who actually has a specific scenario where it is a must. – chris Jul 03 '13 at 17:32
  • learn about pimpl idiom so as to use ur already POINT struct in implementation file(.cpp) – cyber_raj Jul 03 '13 at 17:34
  • @Borgleader: The idea behind it is that `setStruct` wouldnt copy only the pointer and not the whole memory. @chris: Is that why there are a lot people writing `_`? How can I avoid using `new`? – andrew Jul 03 '13 at 17:36
  • Copying one pointer or 2 longs is about the same in terms of performance. You're micro-optimizing. – Borgleader Jul 03 '13 at 17:38
  • @Borgleader: This is just an example. In reality there are a few more bytes (about 14 to 45). I know that there is only a short delay but is is more optimized, is it? – andrew Jul 03 '13 at 17:40
  • 1
    Like I said, you're prematurely optimizing and in doing so making your code less obvious. Express your ideas in the simplest most obvious way and if (AND ONLY IF) it turns out to be a bottleneck, then optimize. (Hint: 99% of the time, it won't be) – Borgleader Jul 03 '13 at 17:41
  • @andrew, Any RAII solution is preferred. In this case, a smart pointer. – chris Jul 03 '13 at 19:23
  • I think this was a simplified example for a larger case. Suppose instead of "struct POINT" it was "struct BIGDATA" and contained gigabytes. I think the question was exactly *how* to optimize, not *if*. – Michael Feb 07 '18 at 20:06

2 Answers2

4

In this case you may be better off using inheritance instead of composition. It will eliminate the need to manage an additional resource and allow your "wrapper" to act as a POINT instead of requiring accessors and mutators for the entire POINT structure.

namespace wrapper {
    class Point : public ::POINT
    {
    public:
        Point() { }
        ~Point() { }

        // The following accessors/mutators may not be necessary.
        // They can however be handy with code that requires a pointer to
        // member function (i.e. transformations)
        void setX(long nx) { x = nx; }
        long getX() { return x; }
        void setY(long ny) { y = ny; }
        long getY() { return y; }

        // copy assignment operators
        Point& operator=(const POINT& p)
        {
            x = p.x;
            y = p.y;
            return *this;
        }

        Point& operator=(const Point& p)
        {
            x = p.x;
            y = p.y;
            return *this;
        }
    };
}

If you want to prevent direct access to the members of POINT you can use private inheritance. You can also provide a conversion operator to allow implicit conversions from Point to POINT. This will replace the POINT* getStruct() member function but still allow you to easily use it with functions that require POINT as an argument.

namespace wrapper {
    // Use private inheritance to prevent direct access to the
    // members of POINT
    class Point : private POINT
    {
    public:
        Point() { }
        ~Point() { }

        // Copy constructor
        Point(const ::POINT& p) { x = p.x; y = p.y; }

        // Accessor/mutators
        void setX(long nx) { x = nx; }
        long getX() { return x; }
        void setY(long ny) { y = ny; }
        long getY() { return y; }

        // Allow implicit conversions to POINT* when necessary
        // Replaces getStruct()
        operator ::POINT*() { return this; }
        operator const ::POINT*() const { return this; }

        // Copy assignment operators
        Point& operator=(const POINT& p)
        {
            x = p.x;
            y = p.y;
            return *this;
        }

        Point& operator=(const Point& p)
        {
            x = p.x;
            y = p.y;
            return *this;
        }
    };
}

extern "C" void someCFunction(POINT *);

int main()
{
    POINT cp;
    wrapper::Point p;

    p.x = 0; // FAIL
    p.setX(0); // OK
    p = cp; // OK

    // No need to call getPoint().
    someCFunction(p);
}

Note: I have removed the use of inline as they are unnecessary. Functions defined within a class definition are already inline (see $7.1.2/3). Kudos to Chris for reminding me.

Community
  • 1
  • 1
Captain Obvlious
  • 19,754
  • 5
  • 44
  • 74
  • do you really need the setter/getters? you publicly inherited from a struct so I figure the members will also be public? – Borgleader Jul 03 '13 at 17:43
  • I had considered removing them but decided not to. I added a note on why they _may_ still be useful. It's hard to say since I don't know what else the OP is trying to accomplish with the wrapper. – Captain Obvlious Jul 03 '13 at 17:49
  • This is an nice idea. But I would use `private` instead of `public` for inheritance too. Is there an argument against this? – andrew Jul 03 '13 at 17:50
  • @CaptainObvlious Fair enough. – Borgleader Jul 03 '13 at 17:50
  • @CaptainObvlious: My aim would be creating a class that provides a way for handling with c structs without using direct member calling. Anyway, thanks for this nice code :) – andrew Jul 03 '13 at 17:56
  • @andrew No argument against private inheritance and I have included an additional example using it along with conversion operators that can replace `getStruct()` – Captain Obvlious Jul 03 '13 at 18:11
  • @CaptainObvlious: The second code works very well, thank you :). But the first code allows conversions from `Point` to `POINT`. The second allows only conversions from `Point*` to `POINT*`. Is there a way to convert it "directly"? – andrew Jul 03 '13 at 18:32
  • You can't. Those operators shouldn't have been included in the example, sorry. Conversion operators are never considered when converting to a base class or reference of the base class. The only operators that should have been included were to `POINT*` to replace `getStruct()` – Captain Obvlious Jul 03 '13 at 18:47
  • 1
    The `inline` markings are redundant when inside the class if I'm not mistaken. – chris Jul 03 '13 at 19:24
  • @chris Correct. 7.1.2/3 - A function defined within a class definition is an inline function. I'm having one of _those_ days, thanks for pointing that out. – Captain Obvlious Jul 03 '13 at 19:33
  • To convert `Point` to `POINT` I have to use `Point p; POINT p_ = *(POINT *)p;`. This works fine, but writing this all the time is a bit annoying. Do you think it would be unprofessional, if I would add this member to `Point`? `POINT convert() { return this; }` – andrew Jul 03 '13 at 20:32
  • That will work fine as long as there is no expectation that changes to the returned `POINT` will be reflected in the `Point` object. You can also do `POINT& getPOINT() { return *this; }` – Captain Obvlious Jul 03 '13 at 20:40
  • Is there a way to construct a Point reference from POINT? (or in other words, interpret a pointer to either as the other?) I would expect the 2 to have the same memory allocation order and foot print... – Michael Feb 07 '18 at 20:13
0

As already noted _POINT is a reserved name because it starts with _ + capital letter.

Using all caps for type names is subjective although I tend to steer away from it.

Your class will have all sorts of problems (double delete, delete non-heap memory, and more) if you ever copy it, or pass the address of a stack-based POINT into setStruct.

Your class will be much much simpler and less prone to error if you simply compose by value. You save a level of indirection in exchange for copying a bit more data, but chances are the copy will be better cached than an indirection that may have to hit memory twice.

There isn't any real problem with having getStruct and setStruct functions to translate explicitly to and from the C structure.

However my question here is: What does your C++ wrapper provide you that the C-struct doesn't? You're just providing wrapped methods to the individual attributes anyway rather than some sort of interface that does other operations on the class. In this case I can't actually see a reason to use the wrapper at all (unless you plan to add debugging logic to the getters and setters so you can follow program flow easier).

Mark B
  • 95,107
  • 10
  • 109
  • 188
  • I changed `POINT_` to `_POINT`. How can I avoid this problems. Could you please describe it more detailed and give some examples? – andrew Jul 03 '13 at 17:44
  • Thanks for the edit. Yes I want to provide more functionality :). – andrew Jul 03 '13 at 17:46