1

I would like to list all permutations of n numbers. Until now everything seems normal but I encountered a very strange behaviour. With this code:

int **liste_permutations(int n){
    int i, fact = factorielle(n);
    int **tab=malloc(sizeof(int*)*fact);
    for(i=0; i<fact; ++i)
    {
            tab[i] = malloc(sizeof(int)*n);
    }
    for(i=0;i<n;++i)
    {
            tab[0][i] = n-i;
    }

    for(i=1;i<fact;++i)
    {
            tab[i] = next_permutation(tab[i-1], n);
    printf(" ");
    }
    return tab;}

The output of this main()

    int **tab;
    tab = liste_permutations(3);
    for(i=0; i<factorielle(3); ++i)
    {

            for(j=0; j<3; ++j)
            {
                    printf("%d", tab[i][j]);
            }
            printf("\n");
    }

is

          321
   231
   213
   312
   132
   123

but if I change it to

int **liste_permutations(int n){
    int i, fact = factorielle(n);
    int **tab=malloc(sizeof(int*)*fact);
    for(i=0; i<fact; ++i)
    {
            tab[i] = malloc(sizeof(int)*n);
    }
    for(i=0;i<n;++i)
    {
            tab[0][i] = n-i;
    }

    for(i=1;i<fact;++i)
    {
            tab[i] = next_permutation(tab[i-1], n);
    }
    return tab;}

the output of the main is :

321
231
321
231
321
231

And if I try to do this with n=5 for exemple, the output is blank (probably because it try to output 125 " ")

here is the next_permutation code :

int *next_permutation(int *t, int n){
    //printf("n = %d\n", n);
    int i, max, count;
    for(i=0;(i<n) && (max !=i); ++i)
    {
            if(t[i] == n)
            {
                    max = i;

            }
            if(t[i] == (t[i-1]+1))
            {
                    ++count;
                    if(count == (n-1))
                    {
                            return NULL;
                    }
            }

    }
    //printf("max = %d\n", max);
    if(n==1)
    {
            //printf("n=1\n");
            return NULL;
    }
    int *next = malloc(n);
    if(max == n-1)
    {
            //printf("max == n-1\n");
            int *s;
            s = malloc(sizeof(int));
            for(i=0; i<(n-1);++i)
            {
                    s[i]=t[i];
            }
            for(i=0; i<n-1; ++i)
            {
                    //printf("%d", s[i]);
            }
            //printf("\n");
            s = next_permutation(s, n-1);
            if(s == NULL)
            {
                    //printf("NUUUUUUl");
            //        next = NULL;
                    return NULL;
            }
            //printf("reprise en n = %d\n", n);
            for(i=1;i<n;++i)
            {
                    next[i] = s[i-1];
            }
            //printf("\n");
            free(s);
            next[0]=n;
            return next;
    }
    else
    {
            //printf("max != n-1\n");

            for(i=0; i<n; ++i)
            {
                    next[i] = t[i];
            }
            int tmp = next[max];
            next[max] = next[max+1];
            next[max+1] = tmp;
            for(i=0;i<n;++i)
            {
                    //printf("%d", next[i]);
            }
            //printf("\n");
            return next;
    }}

EDIT : modified what the 2 first comment said, but I still have the sam issue.

EDIT2 : Thank you to every body who helped me ! espescially to mweerden who showed me the right path (it was because count was uninitialized) !

3
  • 1
    Your next_permutation code is leaking memory right and left, not to mention all the initial allocations in liste_permutations: they are also gone after re-assignment. Rather than allocating a new array each time, you should pass an existing array for the result, i.e. next_permutation(tab[i], tab[i-1], n) Commented Aug 13, 2016 at 10:05
  • You want to trace the code using a debugger to learn what is really going on. Commented Aug 13, 2016 at 10:07
  • 1
    As you are new here I figured giving you a little hint. If one of the answers solved your problem you should accept that answer as the correct answer. To accept an answer you can click the little check mark below the vote buttons. It both increases your reputation and the reputation of the person who helped you. Commented Aug 13, 2016 at 11:56

3 Answers 3

2
tab[i] = malloc(sizeof(int*)*n);

tab[i] wants an array of ints (not an array of int*s)

Change to

tab[i] = malloc(sizeof(int)*n);

Also

int *s;
for(i=0; i<(n-1);++i)
{
    s[i]=t[i];
}

You don't reserve space for s (used uninitialized)

Sign up to request clarification or add additional context in comments.

1 Comment

Thank you, I changed it but is still has the same behaviour... Anyway thank you, I just earned a better note because of you !
2

The reason the printf statement has this effect is because you are using a variable max that hasn't been initialised:

int i, max, count;
for(i=0;(i<n) && (max !=i); ++i)

When you don't initialise it, its value will be whatever was left there by other code when that piece of memory was last used. If you don't have the printf statement, the value will be whatever the last call to next_permutation left there. If the printf is there, the value will be some value that is left by the call to printf. Except for the first call to next_permutation, there the value is a leftover from malloc.

Comments

1

You have a number of things wrong here. You start well:

int **tab=malloc(sizeof(int*)*fact);

But this is wrong:

for(i=0; i<fact; ++i)
{
    tab[i] = malloc(sizeof(int*)*n);
}

It should be:

    tab[i] = malloc(sizeof(int)*n); // int, not int *

Then, your next_permutation loop:

for(i=1;i<fact;++i)
{
    tab[i] = next_permutation(tab[i-1], n);
    printf(" ");
}

You've already assigned to tab[i] above - and here you do it again?

Also, in next_permutation itself, you have the following line:

    if(t[i] == (t[i-1]+1))

The problem is that i can be 0 - indexing t[-1] is NOT what you want to do!

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.