Is my function failproof?

Question

I am implementing a list structure in C. My current function I am working on is a destructive append function that takes two lists and appends the second one onto the end of the first. Right now, it appears to be working without runtime errors and memory leaks and I think I have all my cases covered, but when I submit it to my testing server, it says it is not correct. I was wondering if anyone could see any flaws that could cause it to not do what I expect it to do.

The append function:

ilist iappend_destroy(ilist il1, ilist il2){
   if(il1 == NULL && il2 == NULL){
      free(il1);
      free(il2);
      return NULL;
   }else if(il1 == NULL){
      free(il1);
      return(il2);
   }else if(il2 == NULL){
      free(il2);
      return(il1);
   }else{

   ilist tmp = iempty();
   ilist clone = il1;

   while(il1 != NULL){
      tmp = icons_destroy(il1->first, tmp);
      il1 = il1->rest;
   }

   ilist tmpclone = tmp;

   while(tmp != NULL){
      il2 = icons_destroy(tmp->first, il2);
      tmp = tmp->rest;
   }

   idelete(tmpclone);
   idelete(clone);
   return il2;
   }

The declaration of the ilist structure:

struct ilist_ADT{
    struct ilist_ADT *rest;
    int first;    
};

The icons_destroy function:

ilist icons_destroy(int in, ilist il){
   if (il == NULL) {
      ilist anewlist = malloc(sizeof(struct ilist_ADT));
      anewlist->first = in;
      anewlist->rest = NULL;
      return (anewlist);
   } else {
      ilist previous = malloc(sizeof(struct ilist_ADT));
      previous->first = il->first;
      previous->rest = il->rest;
      il->first = in;
      il->rest = previous;
      return il;
   }
}

Answer 1

Your temporary variable names are horrible and give a false impression of what they are doing.

ilist tmp = iempty();   // Is this really a temporary and a temporary of what.
                       // Looks like you are building a copy of il1 onto this list.

ilist clone = il1;      // This is not a clone of il1
                       // It is basically a pointer at the same head node as il1.
                       // I would have called this one il1Iter or loop1 then changed this
                       // value in the loop below thus leaving il1 pointing at the head.

ilist tmpclone = tmp;   // Same comment as for clone.

// Looks like you are copying the copy you just made onto the end of il2.
// This seems like a waste as you are doing two copies of each node.
while(tmp != NULL){
    il2 = icons_destroy(tmp->first, il2);
    tmp = tmp->rest;
}
// A deeper inspection of icons_destroy() finds that when you are
// actually making a copy of the list in reverse order by always inserting
// to the front.

// Thus copying il1 (to reverse it) and then copying it again (to put it back
// in the correct order) with il2 actually puts a copy of il1 in-front of il2.
// Not a very obvious way of implementing the append and as such I would
// suggest you find a way to re-write that so it is obvious what is happening.


// Seems correct but because of the bad naming convention it is hard to tell
// we actually need to go back and read the rest of the code to make sure this
// is correct in context.
idelete(tmpclone);
idelete(clone);