Thread: Error: assignment of i_value in read-only object

  1. #16
    Registered User
    Join Date
    Jul 2020
    Posts
    28
    Unfortunately there is still a logic flaw with the add_record function.

    While the function no longer crashes.

    The add_record function continues to update the head of the record list each time it is called, rather than the head of the record_list the first time a record is added to a key and then subsequent records thereafter.

    The reason that this behaviour is occurring, is that each time the add_record function is called line 94 is being evaluated to true

    Code:
    if (current->record_list == NULL)
    and I have confirmed that this is in fact the case by printing the pointer value at line 91.

    What I don't understand is why lines 100 - 101 don't result in
    Code:
    current->record_list
    becoming a non-NULL value

    Code:
    new_record = (record_t *) malloc(sizeof(record_t));
    current->record_list = new_record;
    Attached Files Attached Files

  2. #17
    Registered User
    Join Date
    Jul 2020
    Posts
    28
    I've added a couple of printf statements after lines 100 - 101, to confirm that new_record and current->record_list pointers are no longer NULL (which is the case).

    But the next time the add_record function is called current->record_list is NULL ??

    No doubt I'm doing something dopey.

    Hopefully I'll manage to figure it out and post back here with the answer myself.

  3. #18
    misoturbutc Hodor's Avatar
    Join Date
    Nov 2013
    Posts
    1,784
    It's quite difficult to keep track of things in my head because of the global variables. There's no reason for the global variables and I think it'd be worthwhile rewriting things to get rid of those global variables completely. As an example (and this might have nothing to do with your problem), I have no idea what the global record_head is for... lines 59 and 78 seem suspicious to me because you're using record_head in assignments but where is record_head (the global) changed to anything other than NULL, and if you do add code to assign it a value other than NULL it's going to me even harder to "trace" the code in your head. So, yeah, maybe look at getting rid of those globals before proceeding

  4. #19
    Registered User
    Join Date
    Jul 2020
    Posts
    28
    Hello Hodor,

    I do appreciate your assistance, however global variables appear to be a legitimate option if you have to share a variable between two or more functions, which is what I plan to do going forward.

    I've had a go at re-writing the existing functions using local variables instead, but that has introduced some other logic issues - with identifying the head of the node for instance - which I will need to figure out how to resolve using only local variables instead.

    While undoubtedly this is possible, given time, it will still leave me with a program design which is basically counter to what I feel will be optimal going forward.

    I've been learning primarly from a book written by Dan Gookin and he uses global variables when using variables across multiple functions. So my view is that global variables are a legitimate development approach - and are not the reason for my current issue.

    I'd prefer to focus my efforts on fixing the existing code rather than essentially starting from scratch.

    Thanks

    VW

  5. #20
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    27,926
    Hodor is right: local variables will make it more easy to reason about your program. Instead of always having to keep a large amount of global state in mind, you only need to worry about a small amount of global state (the state of standard input/output, perhaps), and hence can focus on the particular local state of the function that you're working on.

    It is true that for a trivial program this doesn't matter and it may be faster to write by using global variables, but what's trivial to an experienced programmer may be non-trivial for a beginner, so it is better to begin with good practices like preferring local variables so as to have local state.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  6. #21
    and the hat of int overfl Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    38,170
    > I've been learning primarly from a book written by Dan Gookin and he uses global variables when using variables across multiple functions.
    You're learning a bunch of bad habits.

    The way you solve the problem is by returning results as well as passing parameters.

    > I'd prefer to focus my efforts on fixing the existing code rather than essentially starting from scratch.
    Learning to throw away bad code is also a skill.

    The biggest mistake you're making is copy/pasting the same code 4 times over in your add_record.

    FWIW, no globals.
    Code:
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>
    
    struct record {
        char   c_detail1[20];
        int    i_value;
        struct record *next;
    };
    typedef struct record record_t;
    
    struct node {
        char   key[15];
        record_t *record_list;
        struct node *next;
    };
    typedef struct node node_t;
    
    // NO GLOBALS HERE!!!
    // ------------------
    
    // A separate function to traverse each list.
    // Lesson: Don't try to do too much in one function
    // Lesson: Each function should do one thing, and do it well.
    void print_record_list(const record_t *reclist) {
      while (reclist != NULL) {
        printf("Detail: %s\n",reclist->c_detail1);
        printf("Value:  %d\n",reclist->i_value);
        printf("\n");
        reclist = reclist->next;
      }
    }
    
    void print_key_list(const node_t *keylist) {
      while (keylist != NULL) {
        printf("Key: %s\n", keylist->key);
        print_record_list(keylist->record_list);
        keylist = keylist->next;
      }
    }
    
    node_t *create_node(const char *key) {
      // prepare a new node
      node_t *new = malloc(sizeof(*new));
      strcpy(new->key,key);
      new->record_list = NULL;
      new->next = NULL;
      return new;
    }
    
    // By returning the list, we can write self-starting lists
    // using nothing more than a local variable initialised to NULL.
    // See how it's called in main.
    node_t *add_key(node_t *keylist, const char *key) {
      // prepare a new node
      node_t *new = create_node(key);
    
      if (keylist == NULL) {
        keylist = new;
      } else {
        node_t *tail = keylist;
        while (tail->next != NULL) {
          tail = tail->next;
        }
        tail->next = new;
      }
    
      return keylist;
    } /* add_key */
    
    record_t *create_record(const record_t *data) {
      record_t *new = malloc(sizeof(*new));
      strcpy(new->c_detail1, data->c_detail1);
      new->i_value = data->i_value;
      new->next = NULL;
      return new;
    }
    
    void add_record(node_t *keylist, const char *key, const record_t *data) {
      // prepare new node
      record_t *new = create_record(data);
    
      // find the key
      node_t *current = keylist;
      while( current != NULL && strcmp(current->key,key) != 0 ) {
        current = current->next;
      }
    
      if( current != NULL ) {
        // found the key, now insert the record
        // Lesson: Compare this if/else code with that in add_key
        if (current->record_list == NULL) {
          current->record_list = new;
        } else {
          record_t *tail = current->record_list;
          while (tail->next != NULL) {
            tail = tail->next;
          }
          tail->next = new;
        }
      }
    }    /* add_record */
    
    int main()
    {
        node_t *node_list = NULL;
    
        node_list = add_key(node_list,"Item 1");
        node_list = add_key(node_list,"Item 2");
        node_list = add_key(node_list,"Item 3");
    
        print_key_list(node_list);
    
        record_t *record = malloc(sizeof(*record));
        strcpy(record->c_detail1,"some string");
        record->i_value = 100;
    
        add_record(node_list,"Item 1",record);
        print_key_list(node_list);
    
        return(0);
    }
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.

  7. #22
    Registered User
    Join Date
    Jul 2020
    Posts
    28
    Hello Salem,

    Thanks for responding.

    I must admit I learned a lot more from the various responses to my original question than I expected.

    I was just hoping that someone could help me to understand what the error was that I couldn't see.

    But instead I've learnt that I need to change my approach.

    I'm just starting out with C, so it has been very useful to have learnt this lesson now, rather than later.

    Cheers,
    VW

  8. #23
    Registered User
    Join Date
    Jul 2020
    Posts
    28
    Hello Salem,

    Why is the add_record function definition not the same as add_key in the sense that it returns the (updated) keylist after the function has completed?

    Here is add_key
    Code:
    node_t *add_key(node_t *keylist, constchar*key)
    and it returns the keylist to main after the function has updated the keylist, which I follow.

    But add_record is defined with a void return
    Code:
    void add_record(node_t *keylist, const char *key, const record_t *data)
    I can see that add_record works, but I don't understand why it works?

    I would have thought that like add_key, add_record would need to return the keylist after the function had updated the keylist?

    Thanks

    VW

  9. #24
    and the hat of int overfl Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    38,170
    Well at the moment, add_record doesn't add/remove any of the nodes within the keylist.
    Adding a record to one of the nodes doesn't change the overall arrangement of nodes, so there is no need to return a node pointer.

    However, note line 89:
    > if( current != NULL )

    There is a problem here in that if the key is not found, then data is not saved in your list.
    Worse still, there is no indication back to the caller that the operation failed.

    So there is something for you to fix, make add_record return a success/fail status.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.

  10. #25
    Registered User
    Join Date
    Jul 2020
    Posts
    28
    Hello,

    I have a question in regards to removing a record.

    Once I identify the record to be removed, I'm using the following statement to delete the record (which doesn't work).
    Code:
    free(tail);
    What the free command is presently doing, is deleting / freeing the first member of the record structure;
    Code:
    c_detail1
    rather than the entire record.

    The
    Code:
    i_value
    member is not being deleted / freed.

    Here is an extract of the rem_record function
    Code:
    void rem_record(node_t *keylist, const char *key, const record_t *data) {
    
        // find the key
        node_t *current = keylist;
        while( current != NULL && strcmp(current->key,key) != 0 ) {
        current = current->next;
        }
    
        // found the key, now find the record
        record_t *tail = NULL;
        record_t *prev_record = NULL;
        record_t *temp = NULL;
    
        tail = current->record_list;
    
        // While not at the end of the list and have not matched the record, walkthrough the list
        while (tail != NULL && \
              (strcmp(tail->c_detail1,data->c_detail1) != 0) && \
              (tail->i_value != data->i_value)
              )
        {
            prev_record = tail;
            tail = tail->next;
        }
    
        // null tail means - reached the end of the list - no match, do nothing
        if (tail == NULL)
        {
        return;
        }
    
    
        // handle first record
        if (prev_record == NULL)
        {
            if (tail->next == NULL)
            {
            // only one record
            free(tail);
            tail = NULL;
            return;
            } else  // more than one record
            {
            temp = tail;
            free(tail);
            tail = temp->next;
            return;
            }
        } /* first record */
    }  /* rem_record */

  11. #26
    and the hat of int overfl Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    38,170
    When you delete the last record, you also need to do
    current->record_list = NULL;
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.

  12. #27
    Registered User
    Join Date
    Jul 2020
    Posts
    28
    Quote Originally Posted by Salem View Post
    When you delete the last record, you also need to do
    current->record_list = NULL;
    Hello Salem,

    Thanks for pointing that out.

    Do you have any ideas why
    Code:
    free(tail);
    doesn't delete the entire record?

  13. #28
    Registered User
    Join Date
    Jul 2020
    Posts
    28
    Hello Salem,

    Please ignore my previous post, I found the issue.

    I need to update the record_list pointer, when the first record is deleted, whether there is one record or multiple records in the record list.

    Code:
        // handle first record    
    if (prev_record == NULL)
        {
            if (tail->next == NULL)
            {
            // only one record
            free(tail);
            tail = NULL;
            current->record_list = NULL;   // reset record_list pointer
            return;
            } else  // more than one record
            {
            temp = tail;
            free(tail);
            current->record_list = temp -> next;  // update record_list pointer to 2nd record
            return;
            }
        } /* first record */
    Last edited by VeeDub; 07-27-2020 at 05:25 PM. Reason: code didn't paste correctly

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. How to read 2 adjacents strings object
    By piero borrelli in forum C++ Programming
    Replies: 3
    Last Post: 09-03-2014, 11:54 AM
  2. int assignment to object incompatibility issue - Java
    By Delicieuxz in forum General Discussions
    Replies: 9
    Last Post: 11-26-2013, 01:58 AM
  3. Replies: 7
    Last Post: 12-07-2012, 10:44 PM
  4. Operator Overloading - RHS object is modified by assignment operation
    By IdioticCreation in forum C++ Programming
    Replies: 16
    Last Post: 12-30-2010, 07:33 AM
  5. assignment of read-only location (vectors)
    By jlangfo5 in forum C++ Programming
    Replies: 4
    Last Post: 12-17-2010, 09:20 AM

Tags for this Thread