3
\$\begingroup\$

I was following this tutorial from official python documentation site, but found its examples quite simplistic, so I've built my own wrapper. It does what I intend it to do, but I wonder if this code is good or how it can be improved.

app.h

#pragma once

#include <iostream>
#include <string>

#include <Python.h>


class App {
public:
  App();
  ~App();

  void Run();

private:
  static const std::string PY_CLASS_NAME;
  static const std::string PY_MODULE_NAME;

  static std::string _GetString(PyObject* obj);

  PyObject* _counter;
};

app.cc

#include "app.h"

const std::string App::PY_CLASS_NAME = "Counter";
const std::string App::PY_MODULE_NAME = "lib";

App::App() {
  Py_SetProgramName(Py_DecodeLocale("App", NULL));
  Py_Initialize();
  PyRun_SimpleString("import sys");
  PyRun_SimpleString("sys.path.append(\".\")");

  PyObject* name = PyUnicode_DecodeFSDefault(PY_MODULE_NAME.data());
  PyObject* module = PyImport_Import(name);

  PyObject* consturctor = PyObject_GetAttrString(module, PY_CLASS_NAME.data());
  _counter = PyObject_CallObject(consturctor, NULL);
  std::cout << _GetString(_counter) << "\n";

  Py_XDECREF(consturctor);
  Py_XDECREF(module);
  Py_XDECREF(name);
}

App::~App() {
  Py_Finalize();
}

void App::Run() {
  PyObject* res;
  for (auto i=0; i<10; i++) {
    PyObject_CallMethod(_counter, "Inc", NULL);
    res = PyObject_CallMethod(_counter, "GetValue", NULL);
    std::cout << _GetString(res) << "\n"; 
  }
  PyObject_CallMethod(_counter, "Drop", NULL);
  res = PyObject_CallMethod(_counter, "GetValue", NULL);
  std::cout << _GetString(res) << "\n";
}

std::string App::_GetString(PyObject* obj) {
  PyObject* repr = PyObject_Repr(obj);
  PyObject* str = PyUnicode_AsEncodedString(repr, "utf-8", "~E~");
  const char* bytes = PyBytes_AS_STRING(str);

  Py_XDECREF(repr);
  Py_XDECREF(str);
  return std::string(bytes);
}

lib.py

class Counter(object):
    def __init__(self):
        super().__init__()

        self._c = 0

    def __repr__(self):
        return "Simple Counter" + str(self._c)


    def Drop(self):
        self._c = 0

    def GetValue(self):
        return str(self._c)

    def Inc(self):
        self._c += 1
\$\endgroup\$
  • \$\begingroup\$ Are those supposed to be separate files? If so make it more visible by splitting the different code parts. \$\endgroup\$ – yuri Jun 20 '18 at 17:52
6
\$\begingroup\$

app.h

#pragma once

This is not portable, and does not work as well as advertised. You should use include guards instead.

#include <iostream>
#include <string>

#include <Python.h>

One of the biggest problems currently in C++ is slow compile times. There are several things you can do to lower compile times, but one of the most important ones is to not include what you don't need to include - especially in headers. Obviously that won't be a problem in this small program, but it doesn't hurt to practice best practices.

This header file doesn't need any of these three includes.

  • <iostream>: you don't actually use anything from the <iostream> in the header.
  • <string> : you need this for std::string... but as I'll explain later, you don't actually need any std::string stuff in the header.
  • <Python.h>: because you only use a PyObject pointer in the header, you don't actually need to include it here.

So you can remove all three of these includes from the header, and move them to app.cc, where they're actually needed.

class App {
public:
  App();
  ~App();

Have you heard of the rule of three/five/zero? Since your class requires you to write a destructor, that means it almost certainly requires you to write a copy constructor, move constructor, copy assignment operator, and move assignment operator. And in fact, your class does need you to do these things.

Now you probably don't want your App class to be copyable... that doesn't really make any sense. So for the copy ops all you need is:

App(App const&) = delete;
App& operator=(App const&) = delete;

All that's left is move ops. Should your class be movable? The answer to this question is almost always "yes", unless you have very complex technical reasons why not... which you don't here.

Unfortunately, as written your class's move ops can't be defaulted. You'll have to handle moving manually. The easiest way to do that is to define _counter to be:

PyObject* _counter = nullptr;

And then your move ops are like:

App(App&& other) noexcept
{
    std::swap(_counter, other._counter);
}

App& operator=(App&& other) noexcept
{
    std::swap(_counter, other._counter);
    return *this;
}

But to avoid cleaning up Python multiple times, you need to add a check that _counter is not nullptr in the destructor:

~App()
{
    if (_counter)
        Py_Finalize();
}

That's all you need to make the class safe according to the rule of five.

static const std::string PY_CLASS_NAME;
static const std::string PY_MODULE_NAME;

These don't really need to be class members. They're only used in app.cc, and don't seem necessary anywhere else.

static std::string _GetString(PyObject* obj);

This also doesn't need to be defined in class. It's only used in app.cc. If you move this and the two static constants into app.cc, you don't need to include <string> in the header.

But the main problem with this function is that _GetString is an illegal identifier. You cannot have an identifier that starts with an underscore and a capital letter anywhere. The rules for identifiers are pretty complex, so when I teach I generally advise these simple rules:

  1. Never start with an underscore.
  2. Never use two underscores together.

(_counter is technically okay because it's an underscore followed by a lowercase letter, and because of its scope. But I generally advise using something like counter_.)

app.cc

const std::string App::PY_CLASS_NAME = "Counter";
const std::string App::PY_MODULE_NAME = "lib";

Strings work here, but this is actually a job that might be better handle by string views. String views are constexpr and no-fail - ideal for constants. And the Python library supports non-null-terminated strings, partially. Instead of PyUnicode_DecodeFSDefault() you'd use PyUnicode_DecodeFSDefaultAndSize(). Unfortunately, there's no equivalent for PyObject_GetAttrString(), so you'll need a string there. But at the very least PY_CLASS_NAME can be:

namespace {

constexpr std::string_view PY_CLASS_NAME = "Counter";

} // anonymous namespace

The anonymous namespace is to make sure PY_CLASS_NAME is only visible in app.cc.

On to the constructor:

Py_SetProgramName(Py_DecodeLocale("App", NULL));

This is an issue generally, but you're not doing any error checking here. Py_DecodeLocale() can return nullptr if it fails for a number of reasons. You should really check for that, and handle it - probably by throwing an exception.

There's another issue here: resource management. You're doing everything the old-fashioned C way... mostly because the Python library is a C API. Generally when you're using a C API, you should make some proper C++ RAII classes for resource management.

For example, the memory allocated by Py_DecodeLocale() needs to be freed by a call to PyMem_RawFree()... but you never do that.

The smart thing to do is to create a PyMem_RawFree() deleter:

struct pymem_rawfree_deleter
{
    auto operator()(void* p) noexcept
    {
        PyMem_RawFree(p);
    }
};

Then you can store the program name in a unique_ptr:

namespace {

std::unique_ptr<wchar_t[], pymem_rawfree_deleter> p_program_name;

} // anonymous namespace

And now the memory will no longer leak, because it will automatically be freed.

Now the code becomes something like:

p_program_name.reset(Py_DecodeLocale("App", nullptr));
if (!p_program_name)
    throw std::runtime_error{"failed decoding program name"};
Py_SetProgramName(p_program_name.get());

Also note: NULL is old-fashioned and problematic. Use nullptr.

Py_Initialize();

Another issue with the constructor is that you don't do any checking to make sure it hasn't been called multiple times. What happens if someone does?:

App a1;
App a2;

One answer is that Py_Initialize() will be called twice. This doesn't seem to be a problem, because calling it a second time is guaranteed to do nothing. However, when one of those is destroyed, it will call Py_Finalize()... and now if you try to do anything with the second one, who knows what will happen - probably a crash.

What you'd need to do is make sure that only a single App object is created at a time. You could use a static flag for that... but you kinda already have one with p_program_name. If p_program_name is not nullptr, it must have been set in the constructor, so you know a second App is being constructed. What you can do is test p_program_name first thing in the constructor, and if it is not nullptr, throw an exception. And then you can set it to nullptr in the destructor after the call to Py_Finalize().

PyRun_SimpleString("import sys");
PyRun_SimpleString("sys.path.append(\".\")");

This might benefit from a raw string constant:

constexpr auto PY_INIT_SYS_PATH_CODE = R"~python~(

import sys

sys.path.append(".")

)~python~";

PyRun_SimpleString(PY_INIT_SYS_PATH_CODE);

Next up is more resource management worries:

PyObject* name = PyUnicode_DecodeFSDefault(PY_MODULE_NAME.data());

// ...

Py_XDECREF(name);

The C++ way of doing this is to define a deleter, and use unique_ptr:

struct pyobject_deleter
{
    auto operator()(PyObject* p) noexcept
    {
        Py_XDECREF(p);
    }
};

using pyobject_ptr = std::unique_ptr<PyObject, pyobject_deleter>;

// ...

auto name = pyobject_ptr{PyUnicode_DecodeFSDefault(PY_MODULE_NAME.data())};

And there's no need for all the calls to Py_XDECREF().

Once again, there's no error checking done.

void App::Run() {
    PyObject* res;

It's generally bad practice to declare variables before they're needed. Now you do this so you can reuse res after the loop is done, but there's no real need for this.

for (auto i=0; i<10; i++) {
    PyObject_CallMethod(_counter, "Inc", NULL);
    res = PyObject_CallMethod(_counter, "GetValue", NULL);
    std::cout << _GetString(res) << "\n"; 
}

I'm not 100% sure with the Python API, but I believe you are leaking a lot of stuff here. Each time you call PyObject_CallMethod() it's returning a PyObject*, whether you catch it or not. That PyObject* is nullptr on failure (which you might want to check for) but otherwise it's something you need to consider.

This would be really easy with the pyobject_ptr I showed above:

for (auto i = 0; i < 10; ++i)
{
    auto r1 = pyobject_ptr{PyObject_CallMethod(_counter, "Inc", nullptr)};
    auto res = pyobject_ptr{PyObject_CallMethod(_counter, "GetValue", nullptr)};
    // should at least check res for nullptr
    std::cout << _GetString(res.get()) << '\n';
    // cleanup is automatic
}

And of course, all the same for the bit of code after the loop.

std::string App::_GetString(PyObject* obj) {
    PyObject* repr = PyObject_Repr(obj);
    PyObject* str = PyUnicode_AsEncodedString(repr, "utf-8", "~E~");
    const char* bytes = PyBytes_AS_STRING(str);

    Py_XDECREF(repr);
    Py_XDECREF(str);
    return std::string(bytes);
}

I've already mentioned the problem with the name, but I believe this function has a more serious bug in it. You get the pointer to the string representation of str as bytes, then you decrement the reference count of str... which will probably destroy it. But then you use bytes again... which probably points to freed memory. Normally this would trigger a segmentation fault, but I suspect that Python is managing its memory as a pool, so the OS doesn't detect the use-after-free.

The immediate fix for this is to simply construct the string right away:

std::string App::_GetString(PyObject* obj) {
    PyObject* repr = PyObject_Repr(obj);
    PyObject* str = PyUnicode_AsEncodedString(repr, "utf-8", "~E~");
    const char* bytes = PyBytes_AS_STRING(str);

    auto res = std::string{bytes};

    Py_XDECREF(repr);
    Py_XDECREF(str);

    return res;
}

This will copy the data from bytes right away, before it's released.

But this becomes so much simpler if you use smart pointers:

std::string App::_GetString(PyObject* obj) {
    // Of course, you should be doing error checking....
    auto repr = pyobject_ptr{PyObject_Repr(obj)};
    auto str = pyobject_ptr{PyUnicode_AsEncodedString(repr.get(), "utf-8", "~E~")};
    const char* bytes = PyBytes_AS_STRING(str.get());

    return std::string{bytes};
}

Summary

  • Try to keep headers as minimal as possible:
    • Don't include stuff that isn't necessary.
    • Try to move as much as possible into implementation files especially stuff that's mostly implementation details.
  • Always respect the rule of zero/three/five.
  • Consider reentrancy - what happens when a function is called multiple times or a class is instantiated multiple times.
  • Error checking!!!
  • Use RAII, even if you have to roll your own classes to do it (which you often do when working with C APIs).

And lesser points:

  • Don't use NULL. Use nullptr.
  • Watch out for illegal identifiers. Simple advice: don't start identifiers with an underscore.
| improve this answer | |
\$\endgroup\$

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.