3

I'm working on some legacy C code (normally I work in C#) and I'm having a hard time with one specific string behavior.

The code we have looks something like this:

int no = 0;
const char *values[1000];
for(int i = 0; i < num_of_values; i++) {
  if (/*is invalid*/) continue;
  values[no++] = list_of_objs[i]->value;
}
send_to_display(values, no);

where list_of_objs[i]->value is a const char * and list_of_objs itself is declared as cpp_extern const struct obj_info list_of_objs[] and populated with static data. I believe these are string literals, at this point, and they're used elsewhere in the code as-is, so I can't modify their inital values.

I'm tasked with adding a dynamic prefix to each entry in the values array based on another value. (For the sake of simplicity in the code below, I'm just showing one value - I can easily if or ?: to handle the multiple cases.) In C#, this would be trivial with string concatenation, but I know C's relationship with strings is much more... complex.

My naïve attempt was to declare a single buffer, sprintf into it, and then add that to the values array, but that gave me a list of the final value, repeated i times. (I understand why: reusing the buffer meant that every element in the array was pointed at the same buffer)

int no = 0;
const char *values[1000];
char buf[MAX_STRING_LENGTH];
for(int i = 0; i < num_of_values; i++) {
  if (/*is invalid*/) continue;
  sprintf(buf, "Value: %s", list_of_objs[i]->value);
  values[no++] = buf;
}
send_to_display(values, no);

I've also tried to sprintf directly into the values array, but that gives me a warning-that-should-be-an-error (the const qualifier is discarded).

sprintf(values[no++], "Value: %s", list_of_objs[i]->value);

I know I could declare a buffer in each iteration of the loop and use that, but I worry that that's going to leak memory.

How can I safely get my modified string into this array?

Bobson
  • 13,498
  • 5
  • 55
  • 80
  • 1
    How are the `obj[i]->value` pointers allocated in first place? By `malloc`? – Jabberwocky May 14 '18 at 13:01
  • _adding a prefix to each entry in the array_: in the `values` array? – Jabberwocky May 14 '18 at 13:02
  • @MichaelWalz - As best as I can tell, they're initialized to those values at startup. I just edited `obj` to `list_of_objs`, and added the definition. – Bobson May 14 '18 at 13:08
  • @MichaelWalz - Re: the prefix: Yes. I've clarified that. It's actually one of two strings based on another value, but that's irrelevant to this question - I know how to do that part. – Bobson May 14 '18 at 13:09

2 Answers2

2

You probably need this:

int no = 0;
const char *values[1000];
for(int i = 0; i < num_of_values; i++) {
  if (/*is invalid*/) continue;
  char tempbuffer[100];          // provide enough space for the worst case
  sprintf(tempbuffer, "Value: %s", list_of_objs[i]->value);
  values[no++] = strdup(tempbuffer);
}

but you need to free the pointers in values once you've done with them:

for (int i = 0; i < number_of_values_added; i++)
  free(values[no++]);

If strdup is not available on your platform:

char *strdup(const char *string)
{
   char newstring = malloc(strlen(string) + 1);
   if (newstring)
     strcpy(newstring, string);
   return newstring;
}

Disclaimer: no error checking is done here for brevity.

Disclaimer 2: there may be other ways to resolve the problem, but as it stands here the question is not clear enough.

Your naive attempt and why it is wrong:

char buf[MAX_STRING_LENGTH];  // buf is just a buffer, it's not at all a string in terms oc C#
for(int i = 0; i < num_of_values; i++) {
  if (/*is invalid*/) continue;
  sprintf(buf, "Value: %s", list_of_objs[i]->value);
  values[no++] = buf;  // << you store the same buffer address in all elements of value
}
Jabberwocky
  • 48,281
  • 17
  • 65
  • 115
  • Bleh. That's what I was afraid of, but I appreciate that you show me how to free it afterwards. I presume I can't free it as part of the initial `for` loop, after adding it to `values`, because then it'll be an undefined value when i go to actually iterate through `values` to use it? – Bobson May 14 '18 at 13:11
  • I can't really tell you when to free it because there is not enough information. But for sure: you only need to free stuff that has been returned by `malloc`. – Jabberwocky May 14 '18 at 13:14
  • This doesn't really make sense unless the strings are immutable/cannot be changed and the OP must therefore build up a second data set. Which isn't clear if it's a requirement or not. – Lundin May 14 '18 at 13:15
  • @Lundin yes, it's not very clear, but in one of his comments he mentions that he cannot modify the original strings. – Jabberwocky May 14 '18 at 13:16
  • @MichaelWalz - I've edited the question to add a line to indicate where `values` gets used, and also clarified that there's actually one of several possible prefixes. – Bobson May 14 '18 at 13:23
  • @Jabberwocky - I ended up not using this (I eventually did some refactoring to be able to use a different "write to screen" method), but this answer convinced me that I was approaching it the wrong way. Thanks for the help and the clarity! – Bobson May 23 '18 at 23:31
2

Assuming the code is more or less exactly as you've written, then you never hardcopy the strings, you just point at them and they are allocated elsewhere (maybe they are read-only string literals?). This means that you can't modify it in this particular code, but you must modify it in the source.

So either you have to modify list_of_objs[i]->value by appending the prefix there (using realloc, strcpy etc), or if those strings are to be regarded as immutable, build up a second copy of the data set with the prefix added.

How to do this specifically depends on how the strings are stored in the first place and what you are allowed to modify.


EDIT

For example if you can modify the original code but not the data. I'll assume you have something like this in place:

typedef struct
{
  const char* value;
} obj_t;


int main(void)
{
  const obj_t list_of_objs[2] =
  {
    {.value = "hello" },
    {.value = "world" },
  };
  return 0;
}

You can maintain the code using so-called "X macros" - these are normally to be avoided but they can make lots of sense when maintaining legacy code.

They make the code a bit grim to read, but much easier to maintain. Best of all, the string allocation/concatenation is then done at compile time:

#include <stdio.h>

typedef struct
{
  const char* value;
} obj_t;


#define OBJ_STRINGS_LIST   \
/*  val      prefix:   */  \
  X("hello", "value1: ")   \
  X("world", "value2: ")   \


int main(void)
{
  const obj_t list_of_objs[2] =
  {
    // original initialization as before, ignore prefix
    #define X(val, prefix) {.value = val },
      OBJ_STRINGS_LIST
    #undef X
  };

  const char* prefix_list[2] = 
  {
    // create a look-up table with prefix values based on string literal concatenation
    #define X(val, prefix) prefix val,
      OBJ_STRINGS_LIST
    #undef X
  };

  puts(list_of_objs[0].value);
  puts(prefix_list[0]);

  return 0;
}

Output:

hello
value1: hello
Lundin
  • 195,001
  • 40
  • 254
  • 396
  • I _believe_ the strings are string literals, and they're used elsewhere as-is so modifying them at the source isn't feasible. – Bobson May 14 '18 at 13:14
  • 1
    @Bobson Well, go find out what they are then. If they are string literals, you can build up a prefixed version of them using the pre-processor, at compile time. Much faster than dynamic allocation. – Lundin May 14 '18 at 13:16
  • 1
    @Bobson Added one example how you could modify the code and make it maintainable, without changing the original data - all at compile-time. That's a very C solution though - if you want something more along the lines of C# then go ahead and slaughter the heap memory and the run-time speed :) – Lundin May 14 '18 at 13:43
  • Interesting. I don't know how feasible pre-computing is, but if I can, I do like it better than a lot of temporary buffers. I'll have to dig into this a bit. – Bobson May 14 '18 at 16:15