Question

I am working on a module for a fairly old software package (NWChem) and am unsure about the best way to make a function out of a fairly commonly used pair of function calls. Inside the main package is a set of functions for manipulating a runtime database. A pair that I use together frequently are rtdb_get_info and rtdb_get to pull information about the content of one value in the database and to get the actual value. The headers for these functions are:

extern int rtdb_get_info(const int, const char *, int *, int *, char [26]);
extern int rtdb_get(const int, const char *, const int, const int, void *);

The way they're meant to be called is to do something like the following (ignoring the return values):

int nelem, ma_type;
char date[26];

rtdb_get_info(rtdb, "nameofrtdbvalue", &ma_type, &nelem, date)
rtdb_get(rtdb, "nameofrtdbvalue", ma_type, array)

Where rtdb is an integer that is set somewhere else that designates which database you want to access and array is an array where you want to store the values stored in "nameofrtdbvalue". In some cases you might array = malloc(nelem * sizeof *array) before making the actual rtdb_get call.

Currently, I am using the following function to call this pair:

void rtdb_pull(int rtdb, const char *name, void *array) {

  int ma_type;
  int nelem;
  char date[26];

  if (rtdb_get_info(rtdb, name, &ma_type, &nelem, date)) {
    if (rtdb_get(rtdb, name, ma_type, nelem, array)) 
      printf("Retrieved %s from rtdb.\n", name);
    else 
      printf("%s is in rtdb, but not retrievable.\n", name);
  }
  else 
    printf("Couldn't get info on %s from rtdb.\n", name);
}

Here is a sample way that I will allocate stuff and call rtdb_pull. In this case, I will be pulling an array of 3*npoints doubles from the database and then passing them to some other function that does something with the array coords. Often this will happen inside of a loop where I need to reallocate memory.

double *coords=malloc(sizeof *coords);
int npoints, rtdb;
int i, nloops;
char buf[32];

get_rtdb(&rtdb); //Just something that gets the integer for the runtime database
for (i=0;i<nloops;i++) {
  get_npoints(&npoints);
  coords=realloc(coords,npoints * sizeof *coords);
  rtdb_pull(rtdb,"geometryname",coords);
  use_coords(coords);
}
free(coords);

rtdb_pull is one of the first functions that I wrote when I was learning c and I've noticed that the more I call it, the more likely I am to get segmentation faults, so I want to rewrite it, but am not sure of the best way to do that. It seems like calling it is causing memory leaks, but I'm not sure why. Maybe it's to do with the fact that I'm passing an pointer to memory allocated for an array of doubles to a void *? If so, I'm not sure how to fix that (and continually calling this pair of functions is really annoying).

Any thoughts or suggestions?

Was it helpful?

Solution

Is not wrong to use void* they can work like a generic type when you need and it seems that it's what you are looking for, right?

int myfunc(void* data, int op)
{
  double *mycoords;
  if(op == 1) // a way to control the real type 
    mycoords = (double*) data; 
  //...
  return 0;
}

Inside the function you can cast, or convert the (void*) to the type you need.

Another way to do so is writing a function with variable arguments list, an example from stdarg man page:

#include <stdio.h>
#include <stdarg.h>

void
foo(char *fmt, ...)
{
  va_list ap;
  int d;
  char c, *s;

  va_start(ap, fmt);
  while (*fmt)
    switch (*fmt++) {
      case 's':              /* string */
        s = va_arg(ap, char *);
        printf("string %s\n", s);
        break;
      case 'd':              /* int */
        d = va_arg(ap, int);
        printf("int %d\n", d);
        break;
      case 'c':              /* char */
        /* need a cast here since va_arg only
        takes fully promoted types */
        c = (char) va_arg(ap, int);
        printf("char %c\n", c);
        break;
    }
    va_end(ap);
}

OTHER TIPS

I don't see anything wrong with your rtdb_pull method, and it is perfectly fine to work with void * like that. So maybe the problem is elsewhere :

  • are the rtdb_get_info and rtdb_get rock-solid and well tested ?
  • I don't see a free( coords ); statement after the loop in your code example, is this normal ?

Your call to rtb_get_info updates the value in nelem which from the context I think is the number of elements that is going to be stored into the array. However you size the array is based on the value of npoints, do you know that these values are the same? We can't see how npoints is set.

In addition the get_npoints() function dosn't look right for C code. npoints is declared as an int and I don't see how it is getting updated by the call to get_npoints(), in C++ I'd think it was a reference but this is C.

This statement troubles me:

double *coords=malloc(sizeof *coords);

coords being a pointer to one or more doubles is fine, but you have to allocate the real memory for it somewhere. sizeof *coords means what? You really want to have the argument to malloc() be the total number of bytes you need for this array, and I don't think you're doing that. You also need to cast the returned pointer to double *. Are you using a very old C compiler with poor error detection?

Anyway, start cleaning up your code there, and add some comments throughout your code to explain what you're trying to do.

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top