Implementation of tree with different node types and faux-polymorphism in C

Question

I'm currently learning C by working on my first project. It's a calculator with a parser that transforms input into an operator tree. These trees consist of nodes of different types: Operators (inner nodes), constants and variables. My first attempt was to create a single struct node and just put everything a Node might have in it -- which wastes a lot of memory because not all of the fields are used, depending on the actual type a Node has.

My solution: I implemented three different structs for the three different Node types.

typedef enum
{
    NTYPE_OPERATOR,
    NTYPE_CONSTANT,
    NTYPE_VARIABLE
} NodeType;

typedef NodeType* Node;

struct VariableNode_
{
    NodeType type;
    char var_name[];
};

struct ConstantNode_
{
    NodeType type;
    double const_value;
};

struct OperatorNode_
{
    NodeType type;
    Operator *op;
    size_t num_children;
    Node children[];
};

They all have a type as their first field -- a header to let me know which type a Node actually has. A Node is always a pointer to the heap, so I typedef'ed it to hide its pointer type. A node may be created and used by the following "constructors" and accessors:

Node malloc_variable_node(char *var_name)
{
    VariableNode res = malloc(sizeof(struct VariableNode_) + (strlen(var_name) + 1) * sizeof(char));
    if (res == NULL) return NULL;
    res->type = NTYPE_VARIABLE;
    strcpy(res->var_name, var_name);
    return (Node)res;
}

Node malloc_constant_node(ConstantType value)
{
    ConstantNode res = malloc(sizeof(struct ConstantNode_));
    if (res == NULL) return NULL;
    res->type = NTYPE_CONSTANT;
    res->const_value = value;
    return (Node)res;
}

Node malloc_operator_node(Operator *op, size_t num_children)
{
    if (num_children > MAX_ARITY)
    {
        // Max. arity exceeded
        return NULL;
    }

    OperatorNode res = malloc(sizeof(struct OperatorNode_) + num_children * sizeof(Node));
    if (res == NULL) return NULL;

    for (size_t i = 0; i < num_children; i++)
    {
        res->children[i] = NULL;
    }

    res->type = NTYPE_OPERATOR;
    res->op = op;
    res->num_children = num_children;
    return (Node)res;
}

void free_tree(Node tree)
{
    if (tree == NULL) return;
    if (get_type(tree) == NTYPE_OPERATOR)
    {
        for (size_t i = 0; i < get_num_children(tree); i++)
        {
            free_tree(get_child(tree, i));
        }
    }
    free(tree);
}

NodeType get_type(Node node)
{
    return *node;
}

Operator *get_op(Node node)
{
    return ((OperatorNode)node)->op;
}

size_t get_num_children(Node node)
{
    return ((OperatorNode)node)->num_children;
}

Node get_child(Node node, size_t index)
{
    return ((OperatorNode)node)->children[index];
}

Node *get_child_addr(Node node, size_t index)
{
    return &((OperatorNode)node)->children[index];
}

void set_child(Node node, size_t index, Node child)
{
    ((OperatorNode)node)->children[index] = child;
}

char *get_var_name(Node node)
{
    return ((VariableNode)node)->var_name;
}

double get_const_value(Node node)
{
    return ((ConstantNode)node)->const_value;
}

Is this common practice? Would there be a better way to do it? Would it have been okay to just waste the space and use a single struct as I did before? This would almost always safe me a level of indirection when dealing with trees and transforming them because all Nodes would have the same size and could be recycled. If you want to look at the full file(s): node.h hosted at GitHub

Thanks in advance :)

Edit: Changed ConstantType to double because the type was not defined in the excerpt I posted

Answer 1

Is this common practice?

Enum-based type tracking in C? Yes.

Would there be a better way to do it?

Depends on a few things, including your definition of better. I think this is fine, but if your type-conditional code ends up being extremely long, then you can move to a more C++-style approach, where instead of tracking a type enum, you track function pointers; then your if (get_type()) code disappears and you can blindly call the function pointer. It's a little more complicated, and can have performance implications.

Would it have been okay to just waste the space and use a single struct as I did before?

Again - it depends. How many of these structures are you instantiating? Seven, or seven billion?

Wasting the space can actually be more performant - you can simplify your allocation logic.

There's yet another option where you don't waste space at all - C allows you to do a dirty trick called union punning. Basically, write multiple versions of the structure whose memory overlaps, and choose the right one based on context.

Until performance becomes a very specific and dominant issue, just stick with what you have, which is simple and legible, and resist the urge to prematurely optimize.

Answer 2

Remarks for beginners:

• Never hide a pointer behind a typedef! Never! It makes the code confusing and very hard to read and maintain.
• You can't unfortunately use a generic Node pointer like in your code, because this isn't well-defined by the language. There are ways around it, but they are a bit controversial (see advanced reply below).
• Generally, when doing any kind of polymorphism, you might want to have an "abstract base class". In this case it would have been NodeType, but it should be a struct of its own and each inherited class should contain an instance of the base class as first member. Not a pointer to it.
• Avoid calling functions in the controlling statement of a for loop like you do in for (size_t i = 0; i < get_num_children(tree); i++). This will often produce needlessly slow code. Instead call the function once before the loop, then store the result in a variable.

---

Remarks for veterans (beginners ignore):

One advanced trick which is supposedly fully supported by the standard (but might have diverse quality of implementation on different compilers) is the "union common initial sequence" trick.

There's a rule stating that if you expose a union containing all those structs to the same translation unit as where those structs are used, type punning between the structs is well-defined. In your case something like this:

union type_punning
{
  struct VariableNode_ a;
  struct ConstantNode_ b;
  struct OperatorNode_ c;
};

This union just needs to sit there; you don't need to use it, or even show it to the caller. This tells the compiler that since these struct share a common initial sequence, the NodeType member, we can type pun between them and it is well-defined, as long as we only access shared members of that initial sequence.

Meaning that suddenly we can do this:

void func (struct VariableNode_* vn)
{
  if(vn->type == NTYPE_CONSTANT)
  {
    struct ConstantNode_* cn = (struct ConstantNode_*)vn;
    cn->const_value = something; // well-defined!
  }
}

There's also exceptions to the strict aliasing rule allowing this use.

Source: C17 6.5.2.3

One special guarantee is made in order to simplify the use of unions: if a union contains several structures that share a common initial sequence (see below), and if the union object currently contains one of these structures, it is permitted to inspect the common initial part of any of them anywhere that a declaration of the completed type of the union is visible. Two structures share a common initial sequence if corresponding members have compatible types (and, for bit-fields, the same widths) for a sequence of one or more initial members.