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;
v.run(instructions);
Often these are wrapped in a function. In other places we have things like:
ir_swizzle_swizzle_visitor v;
v.run(instructions);
/* 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:
ir_validate_tree(instructions);
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 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.)
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.
That's a good point. The visitors that we have right now only derive from the base
ir_visitorclass, 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.
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: http://www.boost.org/doc/libs/1_41_0/libs/utility/value_init.htm
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
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.
RTTI, templates, and a couple other things are on the "do not touch" list.
Not at all true. To use a copy constructor you have to know the (derived) type of the thing you're creating. With a
clonemethod, you don't. There are a lot of places where we have anir_rvaluepointer, but we have no idea what the actual class is. Just doingsome_rvalue->clone()just works.Probably true about the initializers.
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 viaglGetInfoLog.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...Why not overload operator()? Doing so should enable
ir_visitor_foobar v; v(instructions);
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.
Well, that's 'homegrown' for me...