Separate rendering thread with XLib (GLX) and OpenGL

I am writing a prototype for something at work. Not very many people here know much about Linux/Unix or Xlib, so I can't really ask here at work (most developers are expert Windows programmers). With this said, I have one class and a driver that I would like feedback on how well I have integrated threads, XLib and OpenGL.

The order of the files below is:

• glx_data.h

• linux_main.cpp

• map.h

• map.cpp

• Makefile

// 1. glx_data.h
#ifndef GLX_DATA_
#define GLX_DATA_
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 

typedef
struct glx_data
{
  Display *dpy;
  Window win;
  XVisualInfo *vi;
  Colormap cmap;
  XSetWindowAttributes swa;
  GLXContext cx;
  XEvent event;
  int dummy;
  bool double_buffer;
} GLXDATA;

#endif//GLX_DATA

linux_main.cpp

```
#include "glx_data.h"
#include "map.h"

static int snglBuf[] = { GLX_RGBA, GLX_RED_SIZE, 1, GLX_GREEN_SIZE, 1,
GLX_BLUE_SIZE, 1, GLX_DEPTH_SIZE, 12, None };

static int dblBuf[] = { GLX_RGBA, GLX_RED_SIZE, 1, GLX_GREEN_SIZE, 1,
GLX_BLUE_SIZE, 1, GLX_DEPTH_SIZE, 12,
GLX_DOUBLEBUFFER, None };

//Global Data
static Map *gsp_Map;

static GLXDATA glxdata;

bool init_instance() {
glxdata.dpy = XOpenDisplay(NULL);
if (glxdata.dpy == NULL) {
fprintf(stderr, "Failed to get XDisplay\n");
return false;
}
if (!glXQueryExtension( glxdata.dpy,
&glxdata.dummy,
&glxdata.dummy)) {
fprintf(stderr, "X server has no OpenGL GLX extension");
return false;
}
glxdata.vi = glXChooseVisual( glxdata.dpy,
DefaultScreen(glxdata.dpy),
dblBuf);
if (NULL == glxdata.vi) {
glxdata.vi = glXChooseVisual( glxdata.dpy,
DefaultScreen(glxdata.dpy),
snglBuf
);
if (NULL == glxdata.vi) {
fprintf(st

I would clean up the makefile

You don't want to do it separately for each file as you then get cut/paste errors.

You want to generalize this as much as possible (so that any change only needs to be done in one place (not in multiple locations).

PS. Not tested.

CXX        = g++
CXXFLAGS   += -Wall -pthread 

TARGET      = linux_main
SRC_FILES   = $(wildcard *.cpp)
OBJ_FILES   = $(patsubst %.cpp,%.o,$(SRC_FILES))

all: $(TARGET)

$(TARGET): $(OBJ_FILES)
    $(CXX) -o $(TARGET) $* $(CXXFLAGS) -lGL -lXext -lX11 -lGLU

clean:
    rm -f $(OBJ_FILES) $(TARGET)

The main():

In C++ we try and use objects and RAII to make the code exception safe.

Any time you have init() followed by a dest() pair it is an indication that you should have wrapped in a class and done the init() in the constructor and the dest() in the destructor.

Also note you never actually initialize gsp_Map so if it fails to initialize you end up deleting an invalid pointer.

int 
main(int argc, char *argv[]) {
  if (!init_instance()) {
    fprintf(stderr, "Failed to initialize application instance\n");
  }

  message_loop();

  if(gsp_Map) {
    delete gsp_Map;
  }
}

This would have been much neater to implement as:

int main(int argc, char *argv[])
{
     try
     {
         // Move the code for init_instance() into the Map constructor
         // Make sure the destructor tides up correctly.
         Map  mapInstance;

         // Try to reduce the amount of global accessible state.
         // It makes testing really hard to do well. Instead pass objects as
         // parameters if the function needs to use it.
         message_loop(mapInstance);
     }
     catch(...)
     {
         fprintf(stderr, "Failed to initialize application instance\n");
     }
}

In C++ code there is no need for the typedef here:

typedef
struct glx_data
{
  Display *dpy;
  Window win;
  XVisualInfo *vi;
  Colormap cmap;
  XSetWindowAttributes swa;
  GLXContext cx;
  XEvent event;
  int dummy;
  bool double_buffer;
} GLXDATA;

Each of the pointers in this structure are initialied by a function call that obviously allocates so object and returns a pointer. I don't see any code that correctly tides this up. You should wrap each of these pointers in an class that will correctly tidy itself (otherwise at each failure point you need to write code to tidy up the object correctly).

These constructors in Map are obviously not doing anything:

Map::Map()
{
}

Map::Map(Map ©)
{
}

Since you have a constructor that does real work:

Map::Map(GLXDATA *glxdata, int width, int height)

You should delete the useless ones above they are dangerous at best (you should probably make the copy constructor and assignment operator private until you work out what is happening with all the pointers flying around your code).

The destructor is useless. Delete it and let the default compiler version do its job until you have some actual work to do here.

Map::~Map()
{
}

Yoda style testing was popular in the 80/90 but has since been discarded as untatural to read.

if (NULL == glxdata.vi)

// Most people find it easier to read the other way around

if (glxdata.vi == NULL)

There is no real advantage to the Yoda style (it is trying to protect you from accidental assignment) as all compilers will warn you if you accidentally do an assignment in a test and since you are compiling with warnings turned on and striving to have zero warning code it should not happen.

if (glxdata.vi = NULL)
              ^^^   Notice the single =

You are using fprint() rather than std::cerr (and your error messages and clean can be better handled by exceptions).

Inside glxdate them members

GLXContext cx;
XEvent     event;

don't look like they should be part of this structrue. They are not used by it and only really manipulated by functions outside the structure.

Member methods declared inside the class declaration are automatically inline. So there is no need to actually specify this:

inline pthread_t &get_pthread() {return m_render_thread;}
    inline void set_update(bool update) {m_update = update;}

Though I will sometimes put one liners in the header file I still usually prefer to put all methods into the source file (let the compiler worry about the inline optimization you have more important things to do).

Technically this is not allowed:

pthread_create(&m_render_thread, NULL, Map::create_pthread, this);

The pthreads library is a C-Library and thus only understands the C ABI. It has no concept of the C++ ABI and thus can not call C++ functions and definitely can not call class methods. This just happens to work on some systems as you are getting lucky that that the ABI for static member methods is the same as the C-ABI.

Best to just make a C-function so that you are guaranteed for it to work. This C function can call your static member in a valid fashion.

```
extern "C" void create_pthread(void

StackExchange Code Review Q#7396, answer score: 3