Several coworkers and I have been working on a compiler project for the last six months or so. We're implementing it in C++, and, for most of us, it's our first real C++ project. We're using various flavors of visitors1 for a lot of our IR processing. In quite a few places we have visitors that just do stuff but have no value. To invoke them some code that looks something like:

    ir_validate_tree v;;

Often these are wrapped in a function. In other places we have things like:

    ir_swizzle_swizzle_visitor v;;

    /* Do something with v.progress */

The latter form is always wrapped in a function that just returns v.progress.

I find the first form to be particularly ugly. Wrapping it in a function makes using it less ugly, but the ugly is still there, lurking in the shadows.

It occured to me today that at least the first form could be "fixed" by using the constructor as the run method. Instead of the code above, there would be a constructor that had the code from the run method, and callers would just do:


Something similar could be done with the second case, but that would look like:

    progress = ir_swizzle_swizzle_visitor(instructions).progress || progress;

My conundrum, as a C++ newb, is whether or not this is a common idiom. Would someone familiar with C++ (at least more familiar than I am) look at that code and know what was going on? Or would they just think the author was a clueless newb?

1: We primarily use hierarchical visitors.

you've got the right idea already

You should really not put any kind of processing in a constructor. There are a number of reasons for this, ranging from "good taste" to readability to compiler optimization behavior to copy behavior to exception safety. From a pure style perspective, the basic idea is that a constructor should do nothing more than fill in the basic necessary member values for the object to be in a known good and usable state. That means initializing member variables, possibly doing some allocations for buffers or member objects, and nothing more. If the object has no members and no parent objects with non-default constructors then you should not write a constructor for your object. Likewise, a destructor should only clean up the object's owned resources, such as file descriptors or allocated memory/objects.

The way a lot of standard library calls work is to wrap it in a function, just like you do for the second case, which is probably the easiest and clearest way to do things. It's not considered to be in bad taste by any means. Some of the Java-school people think that using free-standing functions is bad form, but honestly, let them take their over-priced and low-quality limited-OOP-centric education and shove it. ;) C++ is explicitly multi-paradigm specifically because real code in the real world on interesting applications often benefits from a mix of OOP, procedural, functional, and declarative programming in different parts of the codebase. If you're trying to write code that is procedural in nature then there's little reason not to write a procedural-style function for that code, even if that function uses an object internally as an implementation detail.

That's actually probably the best way to think about things. Your code is only interested in performing some transformation on the tree and getting a scalar result. That operation is most naturally written as a plain ol' (recursive) function. The fact that you're using a C++ class for its vtable and virtual member functions is an implementation detail and not the most abstract or logical API for that operation. It actually makes more sense to write it as a function with the visitor class being an internal thing than to try to expose the class in any way.

(So far as credentials, I've been using C++ for around 17 years, have taught it at the high school level, have worked on C++ compilers and language tools, and use it daily for professional, academic, and hobby programming.)

Comment by Sean Wed 07 Jul 2010 03:39:50 PM PDT
comment 1
I'd suggest not using the constructor. Instead, make a function which constructs an object of that type, runs it, and returns it. Advantage: if you have more than one type of "run" you don't have to pick one.
Comment by Anonymous Wed 07 Jul 2010 05:02:31 PM PDT
comment 3
Now that I've spent time using ruby I wouldn't bat an eyelash at doing that kind of thing. Before I probably would have written a wrapper. Your calling method also avoids problems with passing temporary visitors.
Comment by Greg Wed 07 Jul 2010 05:04:47 PM PDT
would not do it

what happens if you try inherit from one of your visitors? i don't know for sure, but i think, as the code is then called in the base constructor, it runs before the constructor of the inherited class. also, i think i read somewhere that the stack in the constructor does not get cleaned up (eg objects deconstructed) if an exception is thrown.

Comment by fscan Thu 08 Jul 2010 03:12:39 PM PDT
RE: would not do it

That's a good point. The visitors that we have right now only derive from the base ir_visitor class, but it's possible that we could have a deeper hierarchy in the future (though it seems unlikely).

Okay... Sean and fscan have convinced me. Hurray for blogging.

Comment by IanRomanick Thu 08 Jul 2010 04:01:43 PM PDT

I'm watching C++ glsl2 development branch.

A couple of comments:

1) Why do you use talloc? IR code seems to be a poster child for refcounted structures, and in C++ they're easy.

2) Manual downcast functions can be replaced by dynamic_cast, but I guess you want to avoid RTTI for now.

3) 'Clone' methods beg to be rewritten using copy constructors.

4) Long initializer lists:

glsl_type::glsl_type(GLenum gl_type, unsigned base_type, unsigned vector_elements, unsigned matrix_columns, const char *name) : gl_type(gl_type), base_type(base_type), sampler_dimensionality(0), sampler_shadow(0), sampler_array(0), sampler_type(0), vector_elements(vector_elements), matrix_columns(matrix_columns), name(name), length(0)

You can remove all zero initializers and instead write classes like this: :) And remove explicit bitfields using Boost.Bitfield :) OK, ok. I'm kidding - Boost is too hardcore for most of us.

5) I would have used Boost.Serialization library instead of homegrown IO.

As a hardcore C++ developer I can say, that your code is quite clean and easy to read. But it can be written in about 1.5-2 less lines of code if advanced C++ techniques are used.

PS: I really love structure ir_program :)

Comment by Alex Besogonov Thu 08 Jul 2010 04:23:37 PM PDT
RE: Yeah
  1. Using talloc means that we don't have to do reference counting. By not having to do it by hand we free ourselves for a large category of bugs.

  2. RTTI, templates, and a couple other things are on the "do not touch" list.

  3. Not at all true. To use a copy constructor you have to know the (derived) type of the thing you're creating. With a clone method, you don't. There are a lot of places where we have an ir_rvalue pointer, but we have no idea what the actual class is. Just doing some_rvalue->clone() just works.

  4. Probably true about the initializers.

  5. We're not using homegrown IO. We're using the printf-like routines provided by talloc. We have to keep the compiler logs in a big buffer so that they can be provided to applications via glGetInfoLog.

And thanks for the reminder about ir_program. That was a place holder that was supposed to be removed before we brought our code into the Mesa tree. Oops...

Comment by IanRomanick Fri 09 Jul 2010 09:32:26 AM PDT
operator overloading

Why not overload operator()? Doing so should enable

ir_visitor_foobar v; v(instructions);

Comment by Adrien G. Sat 28 Aug 2010 01:55:36 AM PDT
RE: Yeah

Using talloc means that we don't have to do reference counting. By not having to do it by hand we free ourselves for a large category of bugs.

Why not use smart pointers for that? I have recently tried to benchmark non-threadsafe refcounted smartpointers and talloc, it seems that refcounting is way faster (if a decent malloc/free implementation is provided).

I'm tempted to try to rewrite a part of GLSL compiler to use refcounts and see what happens. I'm also tinkering with OCaml GLSL optimizer, results are interesting so far.

We're not using homegrown IO. We're using the printf-like routines provided by talloc. We have to keep the compiler logs in a big buffer so that they can be provided to applications via glGetInfoLog.

Well, that's 'homegrown' for me...

Comment by Alex Besogonov Mon 30 Aug 2010 11:42:23 AM PDT