Using pycparser to parse C header files

Question

I have a small program which makes uses of pycparser to parse C header files. The code, unfortunately, kinda sprawls out everywhere to handle the different cases (example below).

What's the best way to make this more Pythonic? I thought about case-statements, but those don't exist in Python. Is splitting the function into smaller functions the best approach?

def build_struct(decl):
    """ Recursively builds a structure from external definition.
    """
    _type = type(decl)
    if _type == TypeDecl:
        return build_struct(decl.type)
    elif _type == IdentifierType:
        return " ".join(decl.names)
    elif _type == ID:
        return ['ID', decl.name]
    elif _type == Struct:
        struct = c_types.structureDefinition()
        for d in decl.decls:
            field = build_struct(d)
            struct.add_field(field)
        return struct
    elif _type == Union:
        union = c_types.unionDefinition()
        for d in decl.decls:
            field = build_struct(d)
            union.add_field(field)
        return union
    elif _type == Enum:
        # not implemented yet... but don't raise an exception
        # unsure if there is any value in supporting enums
        return
    else:
        nested = build_struct(decl.type)
        if _type == Decl:
            if decl.bitsize:
                # fields with bitsize defined (i.e. valid:1)
                return c_types.fieldDefinition(decl.name,
                                               int(decl.bitsize.value))
            elif isinstance(nested, c_types.structureDefinition):
                # if it's a structure, assign it's name
                nested.name = decl.name
                return nested
            elif isinstance(nested, c_types.unionDefinition):
                # if it's a union, assign it's name
                nested.name = decl.name
                return nested
            elif isinstance(nested, int):
                # if it's an array, we will just return the total size
                return c_types.fieldDefinition(decl.name, nested)
            else:
                # fields w/o bitsized defined
                id = nested
                # using defined types, like uint32_t
                if id in c_types.size_d:
                    size = c_types.size_d[id]
                # using defined structures, like fast_ip
                elif id in c_types.structs:
                    return c_types.structs[id]
                else:
                    raise c_types.UnknownIdentifier(id)
                # regular fields, i.e. int count;
                return c_types.fieldDefinition(decl.name, int(size))

        elif _type == Typename:  # for function parameters
            raise c_types.NotImplemented
        elif _type == ArrayDecl:
            #raise c_types.NotImplemented
            dimval = decl.dim.value if decl.dim else ''
            id = nested
            # using defined types, like uint32_t
            if id in c_types.size_d:
                size = c_types.size_d[id]
            # using defined structures, like fast_ip
            elif id in c_types.structs:
                return c_types.structs[id]
            else:
                raise c_types.UnknownIdentifier(id)
            return int(dimval) * size
        elif _type == PtrDecl:
            raise c_types.NotImplemented
        elif _type == Typedef:
            id = nested
            # TODO -- this is very common... refactor
            if isinstance(id, c_types.structureDefinition):
                # typedef struct ...
                # TODO -- very similar to code above...
                id.name = decl.name
                return id
            if isinstance(id, c_types.unionDefinition):
                # typedef struct ...
                # TODO -- very similar to code above...
                id.name = decl.name
                return id
            # TODO -- change to c_types.fieldDefinition
            # TODO -- this is very common... refactor
            if id in c_types.size_d:
                # typdef uint32 unsigned long;
                c_types.size_d[decl.name] = c_types.size_d[id]
                return c_types.size_d[decl.name]
            elif id in c_types.structs:
                return c_types.structs[id]
            elif not id:
                # unhandled cases, like enum
                return
            else:
                raise c_types.UnknownIdentifier(id)
        elif _type == FuncDecl:
            raise c_types.NotImplementede

Answer 1

I don't think you're going to fundamentally be able to solve the sprawl, as you have a problem composed of lots of small details. These won't go away no matter how well you organize them, but there are options that help you organize them in ways with lesser algorithmic performance hits.

If you can use a version of python with PEP-443 there are some further alternatives, but assuming you don't have single dispatch support in your python (I believe only alphas/betas have been released yet), or you want to do things manually, it's quite feasible to implement this using a dict mapping types to functions as alternative to the if/elif tree or missing switch statement. The outer one could look something like this:

def build_struct(decl):
    builder = build_struct.type_builder.get(type(decl))
    if builder:
        return builder(decl)

    nested = build_struct(decl.type)
    builder = build_struct.nested_type_builder.get(type(decl))
    if builder:
        return builder(decl, nested)

build_struct.type_builder = {
    TypeDecl: lambda d: build_struct(d.type),
    IdentifierType: lambda d: " ".join(d.names),
    ID: lambda d: ["ID", decl.name],
    Struct: lambda d: build_definition(d, c_types.structureDefinition()),
    Union: lambda d: build_definition(d, c_types.unionDefinition()),
    Enum: lambda d: None, # XXX: let Enums be ignored silently
}

def build_definition(decl, definition):
    for d in decl.decls:
        definition.add_field(build_struct(d))
    return definition

build_struct.nested_type_builder = {
    ...
}

I'm not fully satisfied with the lambdas, even though I chose them for shorter code length. Most of why this code looks shorter is due to not implementing the nested half of your original code. But I do think I prefer the readability for the most part.

Answer 2

I'm having difficulty figuring out what this function does. I stopped concentrating when I reached the else statement and saw another big if block. Nested conditionals like that are too much for my tiny human brain.

The problem is that this function is very low-level - everything that the function does is written out in detail. I'd say about 90% of the lines of code in this function are about how the function does what it does, not what it actually does, even though the what is far more important than the how.

So the goal of improving code (any code, not just this code) should be to make the important points blindingly obvious and brushing the boring, confusing details under the carpet. Bear in mind that you might have to re-read this code in a year's time, once you've completely forgotten what it's about. You'll probably discover that your brain isn't big enough either!

---

With that in mind, the simplest thing you could do to improve this code would to use the 'Extract Method' refactoring (well, 'Extract Function' in this case) on each arm of your big if block. A good rule of thumb is that each arm of a big if block should be one line long. This refactoring is called 'Decompose Conditional'. If you name your extracted methods carefully, the top-level function will be crystal-clear in its intentions.

It's also a good idea to use isinstance, rather than explicitly switching on type, to make your functions more subclass-friendly. If some of the types you're switching on are subclasses of each other, you might need to re-order your elif statements to avoid changing the behaviour of the function. (I hope you have unit tests!)

A brief example of how extracting methods can clean up a function:

def build_struct(decl):
    if isinstance(decl, TypeDecl):
        return build_struct(decl.type)
    if isinstance(decl, IdentifierType):
        return " ".join(decl.names)
    if isinstance(decl, ID):
        return ['ID', decl.name]
    if isinstance(decl, Struct):
        # Since you know what this code does,
        # you can choose better names than these
        # for the extracted functions
        return create_structure_definition(decl)
    if isinstance(decl, Union):
        return create_union_definition(decl)
    if isinstance(decl, Enum):
        return create_enum_definition(decl)
    return build_struct_for_unknown_type(decl)

It's obvious what this function does now! If its argument is a TypeDecl, it recurses on decl.type; if it's a Union then it creates a union definition; if it's some unknown type then it creates a different struct for that, and so on. What's more, you can tell what each extracted function does too - just read the names! This will make it easier to find the code you're looking for when you need to debug it.

---

Finally, it's worth pointing out that if this sort of switch appears more than once in your program then you get to use my personal favourite refactoring, 'Replace Conditional With Polymorphism'. This basically involves moving the repeated conditional logic into a single factory function, which returns different implementations of an interface.

Answer 3

This looks like a good application for the Chain Of Responsiblity pattern.

The essence of a COR is a that you create a lot of little classes ('handlers') that look at a stream of inputs. Each class in the chain checks to see if it can handle the input, and if so it parses it and does what ever it needs to do with it; otherwise it just passes the input along to the next handler in the chain. It's a common way to break up complex parsing tasks into a series of smaller chunks - as the other comments note, this is a fiddly problem with lots of details that will always have lots of nitpicky code in it; however by breaking your parsing tasks into lots of smaller chunks instead a long nested series of ifs or a switch you can make the code much easier to read, maintain and expand.

In the classic COR pattern each handler has a link to the next, and requests are passed along the chain until one handler gets an answer which is passed back up the chain. There's a simpler alternative in which you just loop over a list of handlers until you get a result. The deciding factor between the two approaches would be the need for implementing pre- and or post- answer functioniality. If each handler is atomic, a simple list of parsers will suffice. If handlers have to prepare for or postprocess each other's results then the linked-list method applies.

In your case it looks like all of your code paths are mutually eclusive so I'd stick with the simpler list-of-handler's method

On a structural level it would look like this:

class BaseParser(object):
    SIGNATURE = None
    # in derived classes, this is the match for incoming declaration types

    def handle(self, declaration):
        if declaration != self.SIGNATURE:
            return None
        else:
            return self.parse(declaration)       

    def parse(self, declaration):
        raise NotImplementedError("Override in a derived class")

class IdentifierParser(BaseParser):
    SIGNATURE =  IdentifierType

    def parse(self, declaration):
        return " ".join(declaration.names)

class StructParser(BaseParser):
    SIGNATURE = Struct

    def parse(self, declaration):
        struct = c_types.structureDefinition()
        for d in declaration.decls:
            field = build_struct(d)
            struct.add_field(field)
        return struct

# you'd need to create the handlers for the rest of the code, but you get the idea.
# the parsing proper is handled like so:

HANDLERS = [
IdentifierParser(),
StructParser(),
#... etc.  You would need to watch the order of the parsers only if
# there were conflicting parsers with the same signature -- which would probably
# be best handled with a compound parser with sub-parsers inside it...
]


def parse_declaration(decl):
    result = None
    for handler in HANDLERS:
        result = handler.parse(decl)
        if result: break
    return result

From the looks of your code above this could also be implemented without classes by just creating a list of parsing functions. However you'd then have to retype the signature check logic in all of the functions (and if future requirements change to something where state was relevant, you'd have a harder time refactoring).