5
\$\begingroup\$

I'm wondering if the following Python wrapper for C++ I'm developing is worth expanding upon and including in my portfolio. If so, I plan to add more parameter options and return types. The problem is, that I don't know if my current path is any good. Is my current implementation too amateur? I'm aware there are already libraries for this, but again, this is for fun/experience, and I need more projects in my portfolio. I currently only have one big project and feel I need more. I'm a second-year CS student hoping to apply for my first job in CS soon.

/**
 * Constructor. Initializes m_pyObject to the specified Python class, t_objName.
 */
PyInterface::PyInterface(string t_objName)
{
    char* className = new char[t_objName.length() + 1];
    strcpy(className, t_objName.c_str());

    PyObject * pName, * pModule, * pDict, * pClass;

    Py_Initialize();                                           // Initialize the Python Interpreter
    pName = PyUnicode_FromString((char*)"PythonCode");         // Build the name object
    pModule = PyImport_Import(pName);                          // Load the module object
    pDict = PyModule_GetDict(pModule);                         
    pClass = PyDict_GetItemString(pDict, className);           

    if (PyCallable_Check(pClass))
    {
        m_pyObject = PyObject_CallObject(pClass, nullptr);
    }
    else
    {
        PyErr_Print();
    }
    // Clean up
    Py_DECREF(pName);
    Py_DECREF(pModule);
    Py_DECREF(pDict);
    Py_DECREF(pClass);
    delete[] className;
}

/**
 * Calls Python method, t_method, of class m_pyObject.
 * For use with Python methods that do not return data.
 */
void PyInterface::callPyMethod(string t_method)
{
    char* methodName = new char[t_method.length() + 1];
    strcpy(methodName, t_method.c_str());

    PyObject_CallMethod(m_pyObject, methodName, NULL);                                     // Call specified Python method, t_method.

    delete[] methodName;
}

/**
 * Calls Python method, t_method, with param t_param, of class m_pyObject.
 * For use with Python methods that return a long int.
 * 
 * @return Python long int converted to C++ int.
 */
int PyInterface::callPyMethodInt(string t_method, string t_param)
{
    char* methodName = new char[t_method.length() + 1];
    strcpy(methodName, t_method.c_str());

    string* paramptr = &t_param;                                                           // Create pointer to param.
    PyObject* returnValue = PyObject_CallMethod(m_pyObject, methodName, "(s)", paramptr);  // Python class and method name with string param to be converted and used.

    delete[] methodName;

    return _PyLong_AsInt(returnValue);
}
#ifndef Py_Interface_H
#define Py_Interface_H

#include <Python.h>
#include <string>

using namespace std;

/**
 * C++ class to easily interface with Python classes and methods.
 * 
 * Stores member variable/pointer m_pyObject of type PyObject.
 * m_pyObject is a pointer to the specified Python class and is
 * used in subsequent method calls handled by callPyMethod and
 * callPyMethodInt.
 * 
 * Designed to reduce redundant calls to Python and increase
 * modularity.
 */
class PyInterface
{
public:
    PyInterface(string t_objName);
    void callPyMethod(string t_method);
    int callPyMethodInt(string method, string t_param);

private:
    PyObject* m_pyObject = nullptr;
};

#endif

Do I have something worth building upon? Everything works, but I don't know if this is a suitable format for a wrapper, etc.

\$\endgroup\$

2 Answers 2

6
\$\begingroup\$

I can't answer your questions about your portfolio. It would be best to talk with a professor or student advisor about it, as this will be specific for the school or university you are at. That said, some employers might ask you for examples of your own code, so that might be a reason to keep working on this, as a cross-language utility library will show you have a substantial knowledge about the two languages involved, and how to maintain a library.

There are some issues with your code though:

Avoid using namespace std

It saves you some typing, but it introduces problems, especially if you put using namespace std in a header file: code that includes that header file and doesn't expect this might suddenly not compile properly anymore, as it might change the order in which symbols are looked up. So never do this in header files. I would also just avoid this in the source files, but there it can do less harm.

Copying strings

The way you copy strings is very weird and unnecessary; if you want to access the array of chars that a std::string holds in a non-const way, just use .data() (since C++17), or .front(). But you shouldn't need this, since PyUnicode_FromString() and PyDict_GetItemString() take const char * parameters. Thus:

pName = PyUnicode_FromString("PythonCode");
pClass = PyDict_GetItemString(pDict, t_objName.c_str());

Should just work fine.

Even if you needed to make a mutable copy of the string, avoid manually allocating memory; just copy into another std::string. Also, don't use C functions to copy things, prefer to make use of C++ algorithms like std::copy_n() instead.

Pass strings by const reference where appropriate

You are passing strings by value, however that might involve making a copy. If you don't need to modify the string inside the function, then pass the string by const reference instead:

class PyInterface
{
public:
    PyInterface(const std::string& t_objName);
    ...
};

PyInterface::PyInterface(const std::string& t_objName)
{
    ...
}

The rest of the function doesn't have to be changed.

Write generic code

I plan to add more parameter options and return types. The problem is, that I don't know if my current path is any good.

Consider this: how many parameter options would you need to implement? If you only had to support strings and integers, and wanted to support methods with up to 8 parameters, you'd have to write 512 functions. And then someone comes along and wants to pass floating point numbers, or 9 parameters. This would not be maintainable, and you should therefore find a way to avoid writing many functions.

The solution is to write generic code that can handle any number and any type of parameter. This might be a little harder in C++ than in Python itself, but it can be done using variadic templates. It would look something like this:

template<typename Ret = void, typename... Args>
Ret PyInterface::callPyMethod(const std::string& t_method, const Args&... args)
{
    std::string format = makeFormat(args...);
    PyObject *returnValue = PyObject_CallMethod(m_pyObject, t_method.c_str(),
                                                format.c_str(), 
                                                toParameter(args)...);
    return unpackAs<Ret>(returnValue);
}

And you would call it like so:

PyInterface math("math");
int sum = math.callPyMethod<int>("add", 2, 3);

The above is valid code but it relies on three helper functions that do the heavy lifting:

  • makeFormat() is another variadic template that builds a format string for the given arguments.
  • toParameter() converts one argument to be a suitable type to pass as a parameter to PyObject_CallMethod(), for example by taking the address of an int or by calling c_str() on a std::string.
  • unpackAs<>() does the inverse, and converts the value stored in a PyObject to a C++ type.

This is left as an excercise for the reader, although if you've never done this kind of generic programming in C++ before, it might take a while.

Consider making a PyMethod class

It would be nice if you created a class to wrap a single method, and have it overload the function call operator, so that in the end you can write code like this:

PyInterface math("math");
PyMethod add = math.getMethod("add");
int sum = add(2, 3);

Make sure your Doxygen documentation is correct

It looks like you are using Doxygen to document your classes and functions, which is great! However, make sure you put the documentation as much as possible into the header file, as that is what would be installed on the system if this was a proper library. It is also what a programmer or code editor would look in first to find out about the API for your library.

Also make sure you document all the parameters using @param.

Naming things

Using t_ to prefix parameters and p to prefix a pointer is not too common in C++, but if you are going to use a prefix, use it consistently, otherwise it loses all meaning. For example, className is a pointer to char, so why is there no p in front of it? In the header file, one parameter is named method, without the t_ prefix. I would personally recommend that you don't use any prefixes, except m_ for member variables, as that one brings some real benefits, like avoiding conflicts between function names and parameter names.

You have "Py" in your class and member names. Apart from this becoming noise, it also conflicts with the way types and functions from <Python.h> are named. I recommend that you drop "Py" from your own class and function names, but instead put everything in namespace Python, like so:

namespace Python
{

class Interface
{
public:
    Interface(const std::string& objName);
    void call(const std::string& method);
    ...
private:
    PyObject* object = nullptr;
};

}

The user can then write:

Python::Interface foo("foo");
foo.call("bar");
\$\endgroup\$
1
  • \$\begingroup\$ Regarding function overloading and the PyMethod class, it's PyMethod add that creates the FunctionObject, correct? And then the parameter for it would be the PyObject of the called method? Or am I completely off? This was all highly informative, I appreciate it. Thank you. \$\endgroup\$ Aug 10 at 6:34
2
\$\begingroup\$

Do not cast C-string literals to char *

If necessary, use const char * to avoid the risk of undefined behavior. Related question.

Apply RAII

The obvious issue that makes your code look amateur is ignoring the core C++ RAII concept. Ubiquitous manual resource management is a common beginners mistake.

It has been already mentioned that you should prefer std::string to raw C-strings. In addition, you should pay attention to the PyObject-related resource management. For example,

PyInterface::PyInterface(string t_objName)
{
    /* ... */
    // Multiple uninitialized pointers
    // Undefined behavior, if used before initialization
    PyObject * pName, * pModule, * pDict, * pClass; 

    /* ... */
    // Late initialization
    // Functions acquire resources, but raw pointers do not own resources
    pName = PyUnicode_FromString((char*)"PythonCode");         // Build the name object
    pModule = PyImport_Import(pName);                          // Load the module object
    pDict = PyModule_GetDict(pModule);                         
    pClass = PyDict_GetItemString(pDict, className);           

    /* ... */
    // Manual cleanup must appear in multiple places to ensure
    // that resources are properly released in any execution scenario
    Py_DECREF(pName);  // Does the order matter?
    Py_DECREF(pModule);
    Py_DECREF(pDict);
    Py_DECREF(pClass);
}

The code is fragile and error-prone. Consider alternatives:

// Here, return PyObject by value and pass into functions by reference
PyInterface::PyInterface(const string& t_objName)
{
    Py_Initialize();

    PyObject name = PyUnicode_FromString("PythonCode"); // argument is const char *
    PyObject objModule = PyImport_Import(name);
    PyObject dict = PyModule_GetDict(objModule);
    PyObject objClass = PyDict_GetItemString(dict, t_objName); // pass t_objName by reference

    if (PyCallable_Check(objClass))
        m_pyObject = PyObject_CallObject(objClass, nullptr);
    else
        PyErr_Print();
}

// If Py_DECREF is a function, use it as deleter for std::unique_ptr
PyInterface::PyInterface(string t_objName)  
{
    /* ... */
    auto cleanPyObject = [](PyObject *obj) {
      Py_DECREF(obj);
    };
    using PyObjectPtr = std::unique_ptr<PyObject, decltype(cleanPyObject)> // to simplify the notation
    
    PyObjectPtr pName = {PyUnicode_FromString("PythonCode"), cleanPyObject};
    PyObjectPtr pModule = {PyImport_Import(pName.get()), cleanPyObject};
    PyObjectPtr pDict = {PyModule_GetDict(pModule.get()), cleanPyObject};                         
    PyObjectPtr pClass = {PyDict_GetItemString(pDict.get(), className), cleanPyObject};
    /* ... */
}

The second example is merely a workaround.

Following the code logic, I suspect a resource leak,

int PyInterface::callPyMethodInt(string t_method, string t_param)
{
    /* ... */
    PyObject* returnValue = PyObject_CallMethod(m_pyObject, methodName, "(s)", paramptr);
    /* ... */
    return _PyLong_AsInt(returnValue); // Py_DECREF(returnValue) is missing    
}

To learn more about resource management practices, check the resource management guidelines.

\$\endgroup\$
1
  • \$\begingroup\$ It might be even nicer to write a class that holds a PyObject pointer and deletes it, without having to pass a deleter eplicitly: class Object { PyObject *p; public: Object(PyObject *p): p(p) {} ~Object() { Py_DECREF(obj); } operator PyObject *() { return p; } }; \$\endgroup\$
    – G. Sliepen
    Aug 9 at 21:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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