0

I write a LinkedList class and a test function to test my class. However, I met some problems while compiling and running my program. There are three files: test.cpp LinkedList.cpp and LinkedList.h.

test.cpp:

#include "LinkedList.cpp"
#include <string>
#include <iostream>
using namespace std;

// Loading Array into Linked List for testing
template <class T, size_t n>
void fillArrayIn(LinkedList<T> &list, T (&arr)[n])
{
     unsigned int length = sizeof(arr) / sizeof(*arr);
     for (unsigned int i = 0; i < length; i++)
     {
          list.append(arr[i]);
     }
}

int main()
{

     // Integer LinkedList
     cout << "=====================================" << endl;
     cout << "Test for Integer LinkedList" << endl
          << endl;
     LinkedList<int> intList;

     cout << "Is List Empty? " << (intList.isEmpty() ? "Yes" : "No") << endl
          << endl; // Test isEmpty()

     int intArray[] = {1, 2, 5, 6, 9, 0, 1};
     fillArrayIn(intList, intArray); // Test appending
     cout << "Array Loaded" << endl;
     cout << "Is List Empty? " << (intList.isEmpty() ? "Yes" : "No") << endl;
     intList.printAll();

     intList.insert(*(new int(100)), 3); // Test insertion
     cout << endl
          << "Insert 100 to position 3" << endl;
     intList.printAll();
     cout << "Insert integer outside the List: "
          << (intList.insert(*(new int(100)), 100) ? "Success" : "Fail") << endl
          << endl; // Test illegal insertion

     cout << "Find the position of integre 9: " << intList.positionOf(9) << endl; // Test get the position of element in List
     cout << "Find the position of not existed element: " << intList.positionOf(20) << endl
          << endl; // Test get the position of not existed element

     cout << "The element at position 0 is " << *(intList.get(0)) << endl; // Test get element
     cout << "The element at position 7 is " << *(intList.get(7)) << endl;
     cout << "The element at position 8 is " << intList.get(8) << endl
          << endl;

     cout << "Deleting 100 from list: "
          << (intList.remove(*(new int(100))) ? "Sucess" : "Fail") << endl;
     intList.printAll();
     cout << "Deleting not existed element from list: "
          << (intList.remove(*(new int(20))) ? "Sucess" : "Fail") << endl;
     intList.printAll();
}

LinkedList.cpp:

#include <string>
#include <iostream>
#include "LinkedList.h"
using namespace std;

template <class T>
LinkedList<T>::LinkedList()
{
    head = new Node(NULL);
}

template <class T>
void LinkedList<T>::append(T &_data)
{
    Node *current = head;
    while (current->next)
    {
        current = current->next;
    }
    current->next = new Node(&_data);
    // cout << _data <<" has been appended to the list."<<endl;
}

template <class T>
bool LinkedList<T>::insert(T &_data, const int position)
{
    Node *current = head;
    Node *previous = head;
    for (int i = position + 1; i > 0; i--)
    {
        if (!current->next)
            return false;
        previous = current;
        current = current->next;
    }
    previous->next = new Node(&_data);
    previous->next->next = current;
    return true;
}

template <class T>
T *LinkedList<T>::get(int position)
{
    Node *current = head;
    for (int i = position + 1; i > 0; i--)
    {
        if (!current->next)
            return NULL;
        current = current->next;
    }
    return current->data;
}

template <class T>
bool LinkedList<T>::isEmpty()
{
    if (head->next)
        return false;
    return true;
}

template <class T>
bool LinkedList<T>::remove(const T &_data)
{
    Node *current = head;
    Node *previous = current;
    while (current->next)
    {
        previous = current;
        current = current->next;
        if (*(current->data) == _data)
        {
            previous->next = current->next;
            return true;
        }
    }
    return false;
}

template <class T>
int LinkedList<T>::positionOf(const T &_data)
{
    Node *current = head;
    unsigned int position = -1;
    while (current->next)
    {
        current = current->next;
        position++;
        if (*(current->data) == _data)
            return position;
    }
    return -1;
}

template <class T>
void LinkedList<T>::printAll()
{
    Node *current = head;
    while (current->next)
    {
        current = current->next;
        cout << *(current->data) << "  ";
    }
    cout << endl;
}

template <class T>
class LinkedList<T>::Node
{
  public:
    Node(T *_data) : data(_data) {}
    T *data;
    Node *next;
};

LinkedList.h:

#ifndef LINKEDLIST_H
#define LINKEDLIST_H

template <class T>
class LinkedList
{
  public:
    LinkedList();
    void append(T &);
    bool insert(T &, const int);
    T *get(int);
    bool isEmpty();
    bool remove(const T &);
    int positionOf(const T &);
    void printAll();

  private:
    class Node;
    Node *head;
};

#endif

When I compile my program with g++, it compiled successfully and give out the expected result:

=====================================
Test for Integer LinkedList

Is List Empty? Yes

Array Loaded
Is List Empty? No
1  2  5  6  9  0  1

Insert 100 to position 3
1  2  5  100  6  9  0  1
Insert integer outside the List: Fail

Find the position of integre 9: 5
Find the position of not existed element: -1

The element at position 0 is 1
The element at position 7 is 1
The element at position 8 is 0

Deleting 100 from list: Sucess
1  2  5  6  9  0  1
Deleting not existed element from list: Fail
1  2  5  6  9  0  1

When I try to compile my program with cl (VS2017), the compilation is also successful. However, when I run the program, it will be stuck at a certain point. Running result:

=====================================
Test for Integer LinkedList

Is List Empty? No

Later, I try to create a project in Visual Studio. Surprisingly, this time, it runs without any problem. How can I make it correct to use cl to compile? Thank you if you could give me some hints! I am a new learner in C++.

B.T.W. The object file and executable generated by cl are more than 200KB, which is much more than the ones g++ generated which are around 14KB.

Ronghao
  • 1
  • 2
  • Two compilers give a different result. That means (in almost all cases) that you got lucky with one of them when using something incorrectly. Review your program with an eye on that. If you need help with that you will probably have to make your [mcve] more minimal. – Yunnosch Mar 12 '19 at 05:47
  • When you are done reviewing your program, you can continue with the other helpful proposals here https://ericlippert.com/2014/03/05/how-to-debug-small-programs/ – Yunnosch Mar 12 '19 at 05:47
  • If that does not help try this, by splitting of parts of your total program and getting them as perfect as possible: https://ericlippert.com/2014/03/21/find-a-simpler-problem/ – Yunnosch Mar 12 '19 at 05:48
  • Please check the `Node` constructor and let me know where `next` is initialised. – Yunnosch Mar 12 '19 at 05:56
  • About [using namespace std](https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice)... – Aconcagua Mar 12 '19 at 06:11
  • Off-topic: Forget about `unsigned int length = sizeof(arr) / sizeof(*arr);`, just use the template parameter `n` instead! (Apart from: Why didn't you use `size_t` again?) – a range based loop (`for(auto& e : arr)`) would have been even easier. – Aconcagua Mar 12 '19 at 06:13
  • `bool insert(T &, const int);` – it should be the *reference* that is const, not (necessarily) the index. Would negative indices be meaningfull at all? Well, you might use them to insert from the back (-1 for last position), but for this purpose, I'd rather prefer a separate function. If they are *not* meaningfull, prefer an unsigned type (little bonus: you only will have to check upper range, lower one – `< 0` – naturally is not exceeded...). – Aconcagua Mar 12 '19 at 06:21
  • `LinkedList::LinkedList() { head = new Node(NULL); }` – get used to implementing the constructors initialiser list (not to be confused with `std::initialiser_list`): `LinkedList::LinkedList() : head(new Node(NULL)) { }`; for members of complex types (e. g. std::string, ...), you avoid default initialisation + assignment in favour of direct initialisation with the parameters. Some members (references, non-default-constructible types) *only* can be initialised this way. Prefer C++ *keywords* (`nullptr`) over old (obsolete?) C *macros* (`NULL` – unless targeting pre-C++11 compilers). – Aconcagua Mar 12 '19 at 06:32
  • You've a big design problem by storing pointers to data: `Node(T *_data) : data(_data) { }`. This forced you to `intList.insert(*(new int(100)), 3);`, but now there's a memory leak as your nodes don't delete the data (apart from, the way from pointer to reference back to pointer is really ugly). But if you introduce deletion, you cannot just insert pointers to the array any more (`fillArrayIn`!) as you'd then try to delete memory with local storage duration (UB!). Better approach: Just *copy* or *move* the data into the nodes. Have a look at `std::list` or `std::forward_list` for orientation. – Aconcagua Mar 12 '19 at 06:48

2 Answers2

0

Your check for empty list uses the Node attribute next.

/* ... */
if (head->next)
/* ... */

After using the constructor with pointer parameter for Node, next is not explicitly initialised.

Node(T *_data) : data(_data) {}

These lines are all which are relevant for the first difference of output

Is List Empty? No
Is List Empty? Yes

I tend to be paranoid about initialising and would use the modified constructor with explicit initialisation:

Node(T *_data) : data(_data), next(NULL) {}
Yunnosch
  • 26,130
  • 9
  • 42
  • 54
  • We *cannot* rely on – pointers are *not* initialised implicitly, same as all other basic data types (int, double, etc) are not either. I consider it a weakness of the language, but it is as it is... – Aconcagua Mar 12 '19 at 06:55
  • While I totally agree with you (now and in the deleted part of my answer), I do not get your point. @Aconcagua It seems that you agree with what I wrote... I.e. that is why I am paranoid and do not rely on it... I deleted because it could be mistaken... – Yunnosch Mar 12 '19 at 06:59
  • *[@$\*#!]* – again seem to have missed some update in the standard... If I read [cppreference](https://en.cppreference.com/w/cpp/language/value_initialization) right, since C++11 (or even C+03?) members *are* zero-initialised. Would you read it the same? – Aconcagua Mar 12 '19 at 23:39
  • @Aconcagua In several cases, yes. However if I read it right, none of them apply here, that would at least require a `next()`. And I am not even sure whether `()` == "braces" or braces == `{}`. By the way, I am very impressed by people who can look for an oversight on their own part (take that as a compliment please), but this is not my "defense". Newer standards pass my area of work more or less unnoticed.... ;-) – Yunnosch Mar 13 '19 at 04:56
  • Then thanks for the flowers... `()` and `{}` are *mostly* equivalent, there seem to be some corner cases, though, e. g. references, which apparently are initialised to dangling temporaries with braces (parentheses won't compile), according to a comment of some of our highly decorated (>100k reputation) C++ experts to some question I cannot remember any more... – Aconcagua Mar 13 '19 at 07:20
  • Citing from by link before: *'if T is a class type with a default constructor that is neither user-provided nor deleted [...], the object is zero-initialized and then it is default-initialized if it has a non-trivial default constructor'* Irritating... Not sure if [this section](http://eel.is/c++draft/basic.indet) of the standard is the correct one, don't find a better one, but would conclude that initialisation behaviour actually did *not* change. But if so, we're not talking about paranoia, but necessity! – Aconcagua Mar 13 '19 at 07:53
0

I suggest making the following changes to your code. I don't know whether that will change the behavior but it's worth a try.

  1. Initialize head to nullptr in the constructor of List.

    template <class T>
    LinkedList<T>::LinkedList() : head(nullptr) 
    {
        // Creating a Node with NULL does not make sense to me.
        // head = new Node(NULL);
    }
    
  2. Initialize next to nullptr in the constructor of Node. Letting it be uninitialized can certainly lead to problems.

    Node(T *_data) : data(_data), next(nullptr) {}
    
R Sahu
  • 204,454
  • 14
  • 159
  • 270