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_;
  }
};
}
9
  • 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 3, 2013 at 17:32
  • 2
    _POINT is a reserved 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 3, 2013 at 17:32
  • learn about pimpl idiom so as to use ur already POINT struct in implementation file(.cpp)
    – cyber_raj
    Jul 3, 2013 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 <class>_? How can I avoid using new?
    – andrew
    Jul 3, 2013 at 17:36
  • 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 3, 2013 at 17:41

2 Answers 2

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.

13
  • do you really need the setter/getters? you publicly inherited from a struct so I figure the members will also be public?
    – Borgleader
    Jul 3, 2013 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. Jul 3, 2013 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 3, 2013 at 17:50
  • @CaptainObvlious Fair enough.
    – Borgleader
    Jul 3, 2013 at 17:50
  • 1
    The inline markings are redundant when inside the class if I'm not mistaken.
    – chris
    Jul 3, 2013 at 19:24
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).

2
  • I changed POINT_ to _POINT. How can I avoid this problems. Could you please describe it more detailed and give some examples?
    – andrew
    Jul 3, 2013 at 17:44
  • Thanks for the edit. Yes I want to provide more functionality :).
    – andrew
    Jul 3, 2013 at 17:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.