Access Violation in linked list

[Daniel]

I want to write a method to remove the last node of the linked list.
But the error "Access Violation" exists and the error point to this
method.
What does it means by Access Violation and how can I debug it?
Thanks

void RemoveNode(StepNodePtr pList)
{ StepNodePtr preq, q;
q = pList;
preq = pList;
while (q->next!=NULL)
{ q=q->next;
}

while (preq->next!=q) {
preq = preq->next;
}
preq->next = NULL;
free(q);
}

 

[Kenny McCormack]

I want to write a method to remove the last node of the linked list.

What's a "method" ?

 

[Eric Sosman]

I want to write a method to remove the last node of the linked list.
But the error "Access Violation" exists and the error point to this
method.
What does it means by Access Violation and how can I debug it?

This is Question 16.8 in the comp.lang.c Frequently
Asked Questions (FAQ) list

http://www.eskimo.com/~scs/C-faq/top.html

.... but you can be forgiven for overlooking it because
the question doesn't use the phrase "access violation"
to describe the problem. Trust me, though: this is it.

As to why it occurs, consider what happens if you
ask your function ("function" is the term in C, not
"method") to remove the last node of a one-node list,
or of an empty list.

 

[whisper]

What happens when there is only one node in the list ?
Do you see the problem ?

 

[CBFalconer]

I want to write a method to remove the last node of the linked
list. But the error "Access Violation" exists and the error point
to this method. What does it means by Access Violation and how
can I debug it? Thanks

void RemoveNode(StepNodePtr pList)
{ StepNodePtr preq, q;
q = pList;
preq = pList;
while (q->next!=NULL)
{ q=q->next;
}

while (preq->next!=q) {
preq = preq->next;
}
preq->next = NULL;
free(q);
}

What is a method? C has functions. The Access Violation means you
tried to access memory you had no right to access. Try something
nice an simple:

void removelastnode(StepNodePtr *base)
{ StemNodePtr prev, pprev, root;

prev = pprev = NULL; root = *base;
while (root) {
pprev = prev; prev = root; root = root->next;
}
if (pprev) {
free(prev);
pprev->next = NULL;
}
else if (prev) { /* 1 original item only */
free(prev);
*base = NULL;
}
/* else can't delete from empty list */
} /* untested removelastnode */

To see why, draw some box diagrams. Especially note where root,
prev, pprev point when the while loop is exited. Consider how you
would fix it when the original list consists of no, or only one
item.

You can make a more efficient system that doesn't need the tests
and list walking if all you need to do is add an item or delete the
last. Another very handy routine is listreversal, which can be
done with a single walk.

--
Some informative links:
http://www.geocities.com/nnqweb/
http://www.catb.org/~esr/faqs/smart-questions.html
http://www.caliburn.nl/topposting.html
http://www.netmeister.org/news/learn2quote.html

 

[Daniel]

Is that function OK now? However, is there any better method to do so,
because I think my coding seems quite dirty and long to solve such
simple task.

void RemoveNode(StepNodePtr pList)
{ StepNodePtr preq, q;
int i;
q = pList;
preq = pList;
//CountStep(q) is another function which return the number
//of node in the linked list
i=CountStep(q);
if (i!=0 && i!=1))
{
while (q->next!=NULL)
{ q=q->next;
}

while (preq->next!=q) {
preq = preq->next;
}
preq->next = NULL;
}

if ((CountStep(q)!=0))
free(q);
}

By the way, after I solve the problem of the RemoveNode() function, I
got another error message which is also "Access Violation" from the
another function "isVisited()" However, I think all the cases are
handled already as this is just as simple as the traverse list
function. Could you please kindly tell me the problem of my function?
Thanks

int isVisited(StepNodePtr pList, int x, int y)
{ StepNodePtr p=pList;
while (p!=NULL)
{ if (p->data.x==x && p->data.y==y)
return 1;
p=p->next;
}
return 0;
}

 

[bd]

Is that function OK now? However, is there any better method to do so,
because I think my coding seems quite dirty and long to solve such
simple task.

void RemoveNode(StepNodePtr pList)
{ StepNodePtr preq, q;
int i;
q = pList;
preq = pList;
//CountStep(q) is another function which return the number
//of node in the linked list
i=CountStep(q);

You don't need to count them to know if there's zero or one.
If there's zero, pList will be NULL:

if (pList == NULL)
return;

If there's one, pList->next will be NULL:

if (pList->next == NULL) {
free(pList);
return;
}

if (i!=0 && i!=1))
{
while (q->next!=NULL)
{ q=q->next;
}

while (preq->next!=q) {
preq = preq->next;
}
preq->next = NULL;

The purpose of this is to remove the last node, I surmise? Instead of
finding the last node, then finding the one before that, how about finding
the next-to-last right away?

while (q->next->next) {
q = q->next;
}

free(q->next);
q->next = NULL;

}

if ((CountStep(q)!=0))
free(q);

With your original code, q is the last node, which means it should always be
0, right?

assert(!q->next); /* requires assert.h */
free(q);

}

By the way, after I solve the problem of the RemoveNode() function, I
got another error message which is also "Access Violation" from the
another function "isVisited()" However, I think all the cases are
handled already as this is just as simple as the traverse list
function. Could you please kindly tell me the problem of my function?
Thanks

int isVisited(StepNodePtr pList, int x, int y)
{ StepNodePtr p=pList;
while (p!=NULL)
{ if (p->data.x==x && p->data.y==y)
return 1;
p=p->next;
}
return 0;
}

This function seems okay to me, probably your list was corrupted elsewhere.
That said, you don't need the local variable p, you can just use pList
(though with modern optimizing compilers it probably won't matter)

 

[Herbert Rosenau]

Is that function OK now? However, is there any better method to do so,
because I think my coding seems quite dirty and long to solve such
simple task.

too complicated removed.

Parameter: pointer to anchor

void RemoveNode(StepNodePtr *pList) {
if (!*pList) return; /* list empty, nothing to do */
while (*pList->pNext) /* find the list member _before_ the last one */
pList = pList->pNext;
free(*pList); /* free the last one */
*pList = NULL; /* remove it */
}

To work properly and simple pointer to next must be the the first
member of StepNodePtr.

pList is the pointer to anchor of list (needed to get the list empty)
When Anchor is empty the list contains nothing.
*pList (pList->pNext) is the address of the first element
*pList->pNext (**pList) is the follower of the first element

When your struct is NOT

struct StepNode {
struct StepNode *pNext;
.... content....
}

then you have to modify the function to handle access to the first
element separately.
Having the list at top of the structure simplyfies anything (insert,
remove, walk, sort....)