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?

Improve this question
4
  • 1
    How are the obj[i]->value pointers allocated in first place? By malloc? Commented May 14, 2018 at 13:01
  • adding a prefix to each entry in the array: in the values array? Commented May 14, 2018 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. Commented May 14, 2018 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. Commented May 14, 2018 at 13:09

2 Answers 2

Reset to default
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
}
Improve this answer
Sign up to request clarification or add additional context in comments.

6 Comments

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?
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.
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 yes, it's not very clear, but in one of his comments he mentions that he cannot modify the original strings.
@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.
|
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
Improve this answer

4 Comments

I believe the strings are string literals, and they're used elsewhere as-is so modifying them at the source isn't feasible.
@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.
@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 :)
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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.