Skip to main content
deleted 13 characters in body
Source Link
AndersK
  • 1.4k
  • 6
  • 7

When you declare a class, initialize all member variables

char data = '\0';
size_t freq = 0;
...

(depending on your compiler you may need to do that in the constructor instead)

especially pointers, since a delete on an uninitialized pointer is undefined behavior but a delete on a nullptr is OK (NOP).


void GenerateCode(std::vector<char>& data, std::vector<size_t>& freq, size_t size)

is your intention to modify 'data'? if not, then write const in front

void GenerateCode(const std::vector<char>& data, std::vector<size_t>& freq, size_t size)

why do you pass 'size'? 'data' already has a size: data.size()


you have an unitialized member variable called 'top', you use it in GenerateCode and it will be set there, but if you never call GenerateCode, just a simple class declaration will invoke undefined behavior since delete top is called in HuffManCodes destructor.


memory ownership is not easy to follow in your code

avoid using raw pointers. By using smart pointers ownership is always clear and you dont need to bother about where to delete.

struct Node
{
    std::unique_ptr<Node> left;
    std::unique_ptr<Node> right;  // here it's clear that 'right' owns what it points to
    ...
}

When you declare a class, initialize all member variables

char data = '\0';
size_t freq = 0;
...

(depending on your compiler you may need to do that in the constructor instead)

especially pointers, since a delete on an uninitialized pointer is undefined behavior but a delete on a nullptr is OK (NOP).


void GenerateCode(std::vector<char>& data, std::vector<size_t>& freq, size_t size)

is your intention to modify 'data'? if not, then write const in front

void GenerateCode(const std::vector<char>& data, std::vector<size_t>& freq, size_t size)

why do you pass 'size'? 'data' already has a size: data.size()


you have an unitialized member variable called 'top', you use it in GenerateCode and it will be set there, but if you never call GenerateCode, just a simple class declaration will invoke undefined behavior since delete top is called in HuffManCodes destructor.


memory ownership is not easy to follow in your code

avoid using raw pointers. By using smart pointers ownership is always clear and you dont need to bother about where to delete.

struct Node
{
    std::unique_ptr<Node> left;
    std::unique_ptr<Node> right;  // here it's clear that 'right' owns what it points to
    ...
}

When you declare a class, initialize all member variables

char data = '\0';
size_t freq = 0;
...

(depending on your compiler you may need to do that in the constructor instead)

especially pointers, since a delete on an uninitialized pointer is undefined behavior but a delete on a nullptr is OK (NOP).


void GenerateCode(std::vector<char>& data, std::vector<size_t>& freq, size_t size)

is your intention to modify 'data'? if not, then write const in front

void GenerateCode(const std::vector<char>& data, std::vector<size_t>& freq)

why do you pass 'size'? 'data' already has a size: data.size()


you have an unitialized member variable called 'top', you use it in GenerateCode and it will be set there, but if you never call GenerateCode, just a simple class declaration will invoke undefined behavior since delete top is called in HuffManCodes destructor.


memory ownership is not easy to follow in your code

avoid using raw pointers. By using smart pointers ownership is always clear and you dont need to bother about where to delete.

struct Node
{
    std::unique_ptr<Node> left;
    std::unique_ptr<Node> right;  // here it's clear that 'right' owns what it points to
    ...
}
Source Link
AndersK
  • 1.4k
  • 6
  • 7

When you declare a class, initialize all member variables

char data = '\0';
size_t freq = 0;
...

(depending on your compiler you may need to do that in the constructor instead)

especially pointers, since a delete on an uninitialized pointer is undefined behavior but a delete on a nullptr is OK (NOP).


void GenerateCode(std::vector<char>& data, std::vector<size_t>& freq, size_t size)

is your intention to modify 'data'? if not, then write const in front

void GenerateCode(const std::vector<char>& data, std::vector<size_t>& freq, size_t size)

why do you pass 'size'? 'data' already has a size: data.size()


you have an unitialized member variable called 'top', you use it in GenerateCode and it will be set there, but if you never call GenerateCode, just a simple class declaration will invoke undefined behavior since delete top is called in HuffManCodes destructor.


memory ownership is not easy to follow in your code

avoid using raw pointers. By using smart pointers ownership is always clear and you dont need to bother about where to delete.

struct Node
{
    std::unique_ptr<Node> left;
    std::unique_ptr<Node> right;  // here it's clear that 'right' owns what it points to
    ...
}