0

I've a function :

int* sumCalc(int *p1, int *p2, int n1, int n2)
{
    int sum[100] = {0};// = new int[n1+n2];
    for(int i = 0; i < n1+n2; ++i)
    {
        sum[i] = p1[i] + p2[i];
    }
    return sum;
}

I'm calling this function from main() by writing :

int *sum = sumCalc(p1, p2, n1, n2);

So here I'm gonna get Warning.

To remove warning I changed my int array sum[100] as static and then I'm returning sum.

int* sumCalc(int *p1, int *p2, int n1, int n2)
{
    static int sum[100] = {0};// = new int[n1+n2];
    for(int i = 0; i < n1+n2; ++i)
    {
        sum[i] = p1[i] + p2[i];
    }
    return sum;
}

So is it a good practice to make the local variable static?

And if I to take the memory for array sum[100] from the heap then also I think this error would not be there. But can you tell how use new operator here and initialize all elements of array to zero?

Ayushi bhardwaj
  • 441
  • 5
  • 18
  • That depends whether you want calls to `sumCalc` affecting the previous return values. FWIW, this function is already implemented as `std::transform`. – chris Jul 31 '16 at 19:45
  • @chris: Not quite, transform takes one input range, this one has two. – Mooing Duck Jul 31 '16 at 19:47
  • Do you really need to initialize all the elements to zero? You overwrite them immediately after. – Galik Jul 31 '16 at 19:56
  • 2
    In general, you never want to think of any programming practice as "removing a warning". Understand what the code in question actually does, why it is a problem, what your proposed fix will do, and then think about whether that is what you want. – Nate Eldredge Jul 31 '16 at 19:57
  • 3
    I think a best solution is to return a `std::vector` so you don't need to care about the memory. – Marian Spanik Jul 31 '16 at 20:01
  • 2
    @Galik Worrying about that is a micro-optimization that's unimportant 99.99% of all times (and in this case it makes absolutely no difference anyhow). Zero initialising all memory by default is a rather useful practice. There are much bigger problems with the code really. – Voo Jul 31 '16 at 20:01
  • The biggest problems are a) If `n1+n2 > 100` you write to non allocated memory. b) I assume n1 and n2 refer to the size of the two arrays passed in by pointer. Adding those two together makes no sense - actually if n1 != n2 it's not clear what the code should do anyhow. – Voo Jul 31 '16 at 20:03
  • 1
    @Voo It's not an optimization it's simply not adding needless code for no reason. Zero-initializing memory by default serves no real purpose except maybe to hide the occasional bug for longer. – Galik Jul 31 '16 at 20:09
  • 2
    @Galik Do I really now have to go and find one of the million security bugs that would've been avoided by people zero initializing their locals? Yes, yes the elusive person who never writes a bug doesn't need it, but in real life people make mistakes and this one of those little things that at least diminishes the problems when they happen. – Voo Jul 31 '16 at 20:18
  • 1
    @MooingDuck, It has an overload taking two. Comes in really handy when you need it and don't have `zip`. – chris Jul 31 '16 at 20:35
  • can someone please replace the line `static int sum[100] = {0};` with the `new` keyword.. because when I write `int sum[] = new int[100];` or `int sum[] = new int[100]();` then it gives me error, – Ayushi bhardwaj Jul 31 '16 at 20:51
  • @Ayushibhardwaj `int* sum = new int[n1+n2];` Don't forget to *delete* it with `delete[] sum;` when you are done with it. – Galik Jul 31 '16 at 21:25

2 Answers2

3

The trouble is that in the first version, the variable is allocated off of the stack and not the heap. That means when the function returns, the sum variable disappears, and what you get is a random leftover address that points nowhere.

Yes, declaring the variable as static will make the warning disappear, but it is poor practice. What that tells the compiler is that there is only ever one copy of the sum variable, no matter how many times that the function is called. That means if you call the function again, the array will have the same contents.

int array1[3] = {1, 2, 3};
int array2[3] = {2, 3, 4};

int array3[2] = {5, 6};
int array4[2] = {7, 8};

int* returned = sumCalc(array1, array2, 3, 3); 
int* gotcha = sumCalc(array3, array4, 2, 2); //Now returned is the same array as gotcha!

This can cause confusion because if you were expecting returned and gotcha to be different, you're in for a surprise. They will both be pointed to {12, 14}.

To allocate off of the heap, you have to use the new operator. You can find a number of ways to initialize the array in this question.

Edit: P.S. - Don't forget to delete the pointer allocated with new at the end of the program.

Community
  • 1
  • 1
Brad
  • 2,261
  • 3
  • 22
  • 32
  • The sample code you posted has much bigger troubles than reusing the same array - it will foremost cause quite a lot of out of bound reads (this is really a problem of the design of the function, but is clearly a much bigger issue than the reuse) – Voo Jul 31 '16 at 20:21
  • can you please replace the line `static int sum[100] = {0};` with the new keyword.. because when I write `int sum[] = new int[100];` or `int sum[] = new int[100]();` then it gives me error, – Ayushi bhardwaj Jul 31 '16 at 20:33
  • Ayushi, the line I believe is `int* sum = new int[100]()`. It's not a fixed-length array when you use `new`. – Brad Aug 01 '16 at 19:47
0

The keyword static when used in block scope would make

  • the sum behave like a function variable
  • it will be shared between different invocations of the functions
  • The above may not be desirable.

A work around would be

  • Defining int sum[100] = {0}; inside main

  • Passing a pointer to the same by modifying the sumCalc signature to something like

    void sumCalc(int (*ptr)[100),int *p1, int *p2, int n1, int n2)
// Note the return type is void

and call it like

sumCalc(&sum,p1, p2, n1, n2);

And the loop inside the sum would be :

for(int i = 0; i < n1+n2; ++i)
{
    (*ptr)[i] = p1[i] + p2[i]; 
}
sjsam
  • 21,411
  • 5
  • 55
  • 102
  • can you please replace the line `static int sum[100] = {0};` with the new keyword.. because when I write `int sum[] = new int[100];` or `int sum[] = new int[100]();` then it gives me error, – Ayushi bhardwaj Jul 31 '16 at 20:37
  • If you follow this, you don't need the new keyword at all.. Just go with `int sum[100] = {0};` inside the main – sjsam Jul 31 '16 at 20:43
  • no no I already knew this method to pass the sum array from main() I just wanted to explore more into scope of variables. So I want to use new keyword. – Ayushi bhardwaj Jul 31 '16 at 20:50