0

I am writing an auction program for a class project and one of the features I was trying to implement was a hash table to make searching for auction items by name efficient. I set it up in node format so that you can chain nodes together if their hash value lines up with another item that already exists.

The main problem that I cannot seem to figure out is how some pointer values are changing when I don't think I have done anything to them. I stepped through each line of this program keeping an eye on the Red highlighted areas in the attached screenshots to see when the data changes. In case #1 the data was intact and able to be accessed. However, in case #2 where I simply declare an additional variable (int i = 0;) suddenly the data passed into the function appears to point to a different memory location (0xcccccccc) which from what I understand is another version of null? This is the same no matter what variable type I have tried to declare whether it be an int, const char*, string, etc it all reacts like the second screenshot.

Does anyone know why the program would be doing this? Are there any other troubleshooting tips? Is this a common error and how should I avoid it in the future and for this project?

I can provide a complete code if needed. I appreciate any help you can provide.

Image 1: No additional variable declared, data in tact as expectedImage 1: No additional Variables added, data in tact:

Image 2: integer variable declared, data at ->next suddenly changed. This appears to be this way from the start of the function.enter image description here

Update: I created an MRE as suggested in a comment, the same error can be reproduced using this code.

#include <iostream>
#include <fstream>
#include <string>
#include <vector>
using namespace std;

class AuctionItemBidsMaxHeap {
string name = "test";
public:
const char * getItemName() {
    return name.c_str();
}

};

class AuctionItemHashTable {
private:
struct Node {
    AuctionItemBidsMaxHeap* AuctionItem;
    Node* next = nullptr;
};
Node* itemArray;
int capacity = 50;

int generateHashKey(string auctionItem) {
    return 11;
}

public:
AuctionItemHashTable() {
    //Create the array of X amount of different possible storage locations
    Node emptyNode;
    emptyNode.AuctionItem = nullptr;
    emptyNode.next = nullptr;
    itemArray = new Node[capacity];
    for (int i = 0; i < capacity; i++) {
        itemArray[i] = emptyNode;
    }
}

~AuctionItemHashTable() {
    delete itemArray;
}

void insertItem(AuctionItemBidsMaxHeap* auctionItem) {
    //Check to see if this item already exists
    int key = generateHashKey(auctionItem->getItemName());
    Node newAuctionItem;
    newAuctionItem.AuctionItem = auctionItem;
    newAuctionItem.next = nullptr;
    //Check to see if anything has been inserted there yet
    if (itemArray[key].AuctionItem == nullptr) {
        itemArray[key] = newAuctionItem;
    }
    else {
        //WE have to make room in the semi-linked list
        Node holder;
        holder.AuctionItem = itemArray[key].AuctionItem;
        holder.next = itemArray[key].next;
        newAuctionItem.next = &holder;
        itemArray[key] = newAuctionItem;
    }
}

AuctionItemBidsMaxHeap* getAuctionItem(const char* itemName) {
    int key = generateHashKey(itemName);
    //Loop through all items in location
    Node* currentNode = &itemArray[key];
    if (currentNode == nullptr) {
        return nullptr;
    }
    else {
        if (currentNode->AuctionItem->getItemName() == itemName) {
            cout << "Match" << endl;
        }
        while (currentNode->next != nullptr && currentNode->next != (void*)0xcccccccc) {
            
            int i = 0;
            if (currentNode->next->AuctionItem->getItemName()[0] == 'M') {
                cout << "M Matched" << endl;
            }

            while (currentNode->next->AuctionItem->getItemName()[0] != 'e') {
                //cout << currentNode->next->AuctionItem->getItemName()[i];
            }

            currentNode = currentNode->next;
        }
        //There was an item stored at this location, lets see which one it is
        //void* p = (void*)0xcccccccc;  //Creating a pointer since for some reason my final pointer gets changed to another type of null character upon passing it to a function
        //cout << currentNode->AuctionItem->getItemName() << endl;
        //while (currentNode->next != nullptr && currentNode->next != p) {
            //cout << currentNode->AuctionItem->getItemName() << endl;
            //currentNode = currentNode->next;
        //}
        return currentNode->AuctionItem;
    }
}
};

int main()
{

/**Creating MaxHeap of one bid**/
AuctionItemBidsMaxHeap myBidTest;
AuctionItemBidsMaxHeap myBidTest2;

/**Creating Auction Item Hash Table**/
AuctionItemHashTable auctionItems;
auctionItems.insertItem(&myBidTest);
auctionItems.insertItem(&myBidTest2);
const char* myInput = "test";
auctionItems.getAuctionItem(myInput);
}
marc_s
  • 732,580
  • 175
  • 1,330
  • 1,459
Stonegoblin9
  • 67
  • 1
  • 6
  • Not enough information. Whats likely happening is that you are referencing items on the stack when you create your nodes and they are going out of scope. Show the code where you create your nodes and connect them together. – Mike Vine Dec 09 '21 at 09:37
  • 1
    `Does anyone know why the program would be doing this?` if something like this happens then this is most of the time an indication that you have done something resulting in undefined behavior. Like use-after-free, using initialized variables, out-of-bounds access, dangling references, ... – t.niese Dec 09 '21 at 09:38
  • 1
    I find it rather worrying that you are comparing against `0xcccccc`... – Refugnic Eternium Dec 09 '21 at 09:38
  • 1
    0xCCCCCCCC = uninitialized local (stack) variable, from [here](https://stackoverflow.com/a/370217/2115408) – mmixLinus Dec 09 '21 at 09:38
  • 1
    Without a [mcve] it's hard to tell anything. Make a [mcve] and [edit] your question. While doing so, you might very well find the problem yourself. – Jabberwocky Dec 09 '21 at 09:38
  • Looks like where you are adding `Nodes` to your `itemArray[]` you are not setting `"node->next"` to NULL. – mmixLinus Dec 09 '21 at 09:41
  • it's not null. it is not initialized. adresses like that on Windows are "illegal adress" value for debugging ( it's illegal because unaligned and able to point at char only) . in realse you would not get it more likely. It' not portable feature. you invoke UB zomshow - uninitiazed storage, pointing at automatic storage, etc. – Swift - Friday Pie Dec 09 '21 at 09:49
  • @RefugnicEternium I was just trying things during debugging process :P Did not plan to leave that in there – Stonegoblin9 Dec 09 '21 at 10:14
  • @Jabberwocky I believe I have added in an MRE as requested, is that more acceptable or should I have done something different for future posts? I tried to shorten it as much as possible and eliminate parts of the code that I know work / didn't affect this problem – Stonegoblin9 Dec 09 '21 at 10:15
  • @mmixLinus I believe I set the first item added to each location to have it's -> next set to nullptr. When I insert more nodes after the first, then I have it put them in front and point towards that first node meaning that the end of the chain should always be nullptr, I am pretty sure that is the part you were talking about – Stonegoblin9 Dec 09 '21 at 10:18

1 Answers1

0

First a rant: Why is it that classes still teach pointers in C++? There are MUCH better ways to do this than Node*.

Your code contains several errors, but the most important one is here:

        //WE have to make room in the semi-linked list
        Node holder;
        holder.AuctionItem = itemArray[key].AuctionItem;
        holder.next = itemArray[key].next;
        newAuctionItem.next = &holder; ////<<< ERROR HERE
        itemArray[key] = newAuctionItem;

You create a temporary variable on the stack Node holder; This variable will be destroyed as soon as you leave the function.

But you take a pointer to this variable here

   newAuctionItem.next = &holder;

IOW: Your list contains pointers to objects that no longer exist.

&holder is the address of the variable holder. As soon as holder goes out of scope, the contents of it will be destroyed. But newAuctionItem.next and as a consequence also itemArray[key].next will still point to the memory, where holder used to be.

This is what is called a dangling pointer.

I stopped reading your example, but it is also pretty dangerous to accept pointers to AuctionItems in your insert method. When you are using pointers here, you MUST MAKE SURE, that the actual objects remain valid for as long as they are in the list.

Or, to put it the other way round: You must remove them from your list before they get destructed. And we humans are not made to "make sure". We make errors, so it is better to write code where you cannot make an error like this (i.e. avoid pointers in the first place).


Another error: You are creating an array with itemArray = new Node[capacity];, but you are deleting it with delete itemArray;. When you are using new to create an array, you must use delete[] itemArray to delete it. See here delete vs delete[] operators in C++


A general note: DO NOT USE POINTERS AT ALL (unless you have to). Pointers are an advanced C++ concept.

You could use shared_ptr<> instead. This will take away the burdon of freeing the memory.

For your itemArray you could use std::vector<> instead of allocating an array with new[]; etc...

There are many good and easy to use classes in the C++ library, which will help you a lot writing safer and cleaner code.

Learning C++ is (at least) as much about learning the std Library as about learning the syntax and statements. std::vector<AuctionItemNodes> IS C++.

Hajo Kirchhoff
  • 1,969
  • 8
  • 17