Skip to main content
deleted 775 characters in body
Source Link
G. Sliepen
  • 69.5k
  • 3
  • 75
  • 180

Prevent movingMake it clear that this is a current outside thecircular linked list

In your code for moving the current pointer forward or backwardsTo distinguish it from a regular linked list, I see this:

for (ptrdiff_t i = 0; i < n; i++) {
    list->current   = list->current->next;
    if (list->current == list->head)
        status++;
}

This loop will happily try to go pastmake sure the endnames of the list if n is too large, resulting in a segmentation fault. The check for list->current == list->head is very weird,structs and functions make it will never trigger (if current was list->head,clear that it will have moved past this before this if-statement is triggered), and it doesn't break out of the loopa circular linked list. The code should probably be:

for (ptrdiff_t i = 0; i < n; i++) {
    if (list->current->next)
        list->current = list->current->next;
    else
        return -1;
}

This prevents list->current from becoming NULL, and returns an errorIt's also best if it would have otherwise moved past the end of the list. Note thatfilenames themselves reflect this code still doesn't handle the case when trying to move in an empty list.

Prevent moving current outside the list

In your code for moving the current pointer forward or backwards, I see this:

for (ptrdiff_t i = 0; i < n; i++) {
    list->current   = list->current->next;
    if (list->current == list->head)
        status++;
}

This loop will happily try to go past the end of the list if n is too large, resulting in a segmentation fault. The check for list->current == list->head is very weird, it will never trigger (if current was list->head, it will have moved past this before this if-statement is triggered), and it doesn't break out of the loop. The code should probably be:

for (ptrdiff_t i = 0; i < n; i++) {
    if (list->current->next)
        list->current = list->current->next;
    else
        return -1;
}

This prevents list->current from becoming NULL, and returns an error if it would have otherwise moved past the end of the list. Note that this code still doesn't handle the case when trying to move in an empty list.

Make it clear that this is a circular linked list

To distinguish it from a regular linked list, make sure the names of structs and functions make it clear that it is a circular linked list. It's also best if the filenames themselves reflect this.

deleted 304 characters in body
Source Link
G. Sliepen
  • 69.5k
  • 3
  • 75
  • 180

You have these threetwo functions:

int alx_llist_move_fwd  (struct Alx_LinkedList *list, ptrdiff_t n);
int alx_llist_move_bwd  (struct Alx_LinkedList *list, ptrdiff_t n);
int alx_llist_move_to   (struct Alx_LinkedList *list, ptrdiff_t pos);

They all do the same thing; they move the current pointer. Even though the last one sounds like it is going to an absolute position, it is equivalent to alx_llist_move_fwd()but they take a signed offset and both handle that fine. Just keep a single function:

As already mentioned, alx_llist_move_to(list, pos) makes it sound like you go to an absolute position, but it just moves the current pointer relatively.

Also, alx_llist_edit_current() is probably better rewritten as alx_llist_set_current().

You have these three functions:

int alx_llist_move_fwd  (struct Alx_LinkedList *list, ptrdiff_t n);
int alx_llist_move_bwd  (struct Alx_LinkedList *list, ptrdiff_t n);
int alx_llist_move_to   (struct Alx_LinkedList *list, ptrdiff_t pos);

They all do the same thing; they move the current pointer. Even though the last one sounds like it is going to an absolute position, it is equivalent to alx_llist_move_fwd(). Just keep a single function:

As already mentioned, alx_llist_move_to(list, pos) makes it sound like you go to an absolute position, but it just moves the current pointer relatively.

Also, alx_llist_edit_current() is probably better rewritten as alx_llist_set_current().

You have these two functions:

int alx_llist_move_fwd  (struct Alx_LinkedList *list, ptrdiff_t n);
int alx_llist_move_bwd  (struct Alx_LinkedList *list, ptrdiff_t n);

They do the same thing; they move the current pointer, but they take a signed offset and both handle that fine. Just keep a single function:

alx_llist_edit_current() is probably better rewritten as alx_llist_set_current().

added 1023 characters in body
Source Link
G. Sliepen
  • 69.5k
  • 3
  • 75
  • 180

Prevent moving current outside the list

In your code for moving the current pointer forward or backwards, I see this:

for (ptrdiff_t i = 0; i < n; i++) {
    list->current   = list->current->next;
    if (list->current == list->head)
        status++;
}

This loop will happily try to go past the end of the list if n is too large, resulting in a segmentation fault. The check for list->current == list->head is very weird, it will never trigger (if current was list->head, it will have moved past this before this if-statement is triggered), and it doesn't break out of the loop. The code should probably be:

for (ptrdiff_t i = 0; i < n; i++) {
    if (list->current->next)
        list->current = list->current->next;
    else
        return -1;
}

This prevents list->current from becoming NULL, and returns an error if it would have otherwise moved past the end of the list. Note that this code still doesn't handle the case when trying to move in an empty list.

Prevent moving current outside the list

In your code for moving the current pointer forward or backwards, I see this:

for (ptrdiff_t i = 0; i < n; i++) {
    list->current   = list->current->next;
    if (list->current == list->head)
        status++;
}

This loop will happily try to go past the end of the list if n is too large, resulting in a segmentation fault. The check for list->current == list->head is very weird, it will never trigger (if current was list->head, it will have moved past this before this if-statement is triggered), and it doesn't break out of the loop. The code should probably be:

for (ptrdiff_t i = 0; i < n; i++) {
    if (list->current->next)
        list->current = list->current->next;
    else
        return -1;
}

This prevents list->current from becoming NULL, and returns an error if it would have otherwise moved past the end of the list. Note that this code still doesn't handle the case when trying to move in an empty list.

Source Link
G. Sliepen
  • 69.5k
  • 3
  • 75
  • 180
Loading