2

I have 2 arrays with coordinates and i want to copy them into one array. I used 2 for loops and its working, but i want to know, if i could do it without them, i dont know how to use memcpy in this situation. Here is my code:

int *join(int *first, int *second,int num, int size) {

int *path= NULL;
int i = 0 , j = 0;

path = (int*) malloc (2*size*sizeof(int)); 

    for(i = 0 ; i < num; i++) {
        path[i * 2] = first[i * 2];
        path[i * 2 + 1] = first[i * 2 + 1];

    }

    for(i = num; i < size ; i++) { 
        path[(i*2)] = second[(j+1)*2];
        path[(i*2)+1] = second[(j+1)*2 +1];
        j++;
    }
  return path;
}
mereth
  • 465
  • 3
  • 9
  • 19
  • 2
    [Please see this discussion on why not to cast the return value of `malloc()` and family in `C`.](http://stackoverflow.com/q/605845/2173917). – Sourav Ghosh Dec 02 '16 at 16:45
  • This `path[(i*2)+1]` writes to invalid memory with the last iteration. – alk Dec 02 '16 at 16:48
  • Your memory allocation isn't using the right size. You should be using `(size + num) * sizeof(int)`, shouldn't you — or if there are twice as many entries in each array as the value of `size` or `num`, then multiple by 2 too? As it stands, if the second array is smaller than the first, you trample out of bounds; if the second array is bigger than the first, you waste space (which is much less of a problem). You should check that the allocation succeeded before using the new space; memory allocations do fail. If you get the basics right, then `memcpy(path, first, 2 * num * sizeof(int));`. – Jonathan Leffler Dec 02 '16 at 16:48
  • It is helpful to show an MCVE ([MCVE]) which shows how you call the function and set up the data. It means we have less guessing to do. – Jonathan Leffler Dec 02 '16 at 16:52
  • 1
    @JonathanLeffler: The second loop starts from `num`, `num` is describing the number of `int` pairs in `first`, while `size` describes the total number of pairs in the result (that is, the number of pairs in `first` and `second` combined). The variable names are less than helpful I'll admit. – ShadowRanger Dec 02 '16 at 16:54
  • 1
    @alk: How so? They allocated space for `2 * size` `int`s, so valid indices run from `0` to `2 * size - 1` inclusive. On the last iteration, `i` is `size - 1`, so `i * 2 + 1` is `(size - 1) * 2 + 1` which works out to `2 * size - 1`. – ShadowRanger Dec 02 '16 at 16:57
  • 2
    @ShadowRanger: Hmmm...OK; yes. All the more reason to show how the function is used. A illustrative call like `int *new_arr = join(old_1, old_2, num_old_1, num_old_1 + num_old_2)` would tell us a lot about how it is supposed to work. – Jonathan Leffler Dec 02 '16 at 16:57
  • @ShadowRanger: Right! Sry! Temp brain lapse ... so, quitting for the day. :} – alk Dec 02 '16 at 16:58

1 Answers1

4

Just calculate the correct number of bytes to copy and copy from each origin to the correct offset:

int *join(int *first, int *second, int num, int size) {
    // Compute bytes of first
    const size_t sizeof_first = sizeof(*first) * 2U * num;
    // Computes bytes of second as total size minus bytes of first
    const size_t sizeof_second = sizeof(int) * 2U * size - sizeof_first;

    int *path = malloc(sizeof(int) * 2U * size); 

    // Copy bytes of first
    memcpy(path, first, sizeof_first);
    // Copy bytes of second immediately following bytes of first
    memcpy(&path[2U * num], second, sizeof_second);
    return path;
}
ShadowRanger
  • 143,180
  • 12
  • 188
  • 271
  • @chux: Oops, I had the correct declaration before, but while editing before posting I lost the type in a copy'n'paste error. :-) Fixed. I also moved the `sizeof`s first as suggested; it doesn't match OP's ordering, but no reason not to fix it while I'm at it. – ShadowRanger Dec 02 '16 at 16:59
  • thnaks you all for help, that helped a lot – mereth Dec 02 '16 at 17:01
  • @chux: Heh. I was thinking of that and decided, just to be sure, to make all the `2` literals explicitly `unsigned`, which I believe will guarantee that `&path[2U * num]` works. And yeah, I agree that using `size_t` appropriately is the best solution, but I'm more leery of tweaking the prototype than tweaking the internals. – ShadowRanger Dec 02 '16 at 17:04
  • Agree with concerns about prototype tweaking. `2U * num` is a reasonable improvement over `2 * num`, though not _guaranteed_ to prevent a computation problem with a large positive `num`. `INT_MAX == UINT_MAX` is possible, per C, but likely applies only to esoteric platform in some landfill. – chux - Reinstate Monica Dec 02 '16 at 20:20