[Mesa-dev] [PATCH v2 5/5] i965/gen6: Initial implementation of MSAA.

[Kenneth Graunke]

On 05/14/2012 06:24 PM, Paul Berry wrote:
[snip]
> I'm not going to try to change your mind, but in the spirit of
> clarifying the method that was behind my madness:
>
> I did not have the clarity of mind during our discussion today to point
> out that in creating the brw_blorp_params class, I was in fact trying to
> follow a standard design pattern, the Gang of Four "command" pattern
> (http://en.wikipedia.org/wiki/Command_pattern).  The idea of that
> pattern is to encapsulate all of the information necessary to perform an
> action into a class; this allows the action to be treated as a first
> class object.

Yeah, I wasn't really opposed to making an object that represents the 
command...just the fact that you called it "brw_blorp_params". 
Parameters are just (a collection of) data that you pass to a function 
or method.  You can create a struct of parameters, but that's 
essentially just grouping them together.  They really can't stand alone. 
  Otherwise, it begs the question "parameters to /what/?"

[snip]
> Other alternatives I considered included:
>
> - Make intel_context a class, and make executing a blorp action a method
> on that class, so that last line would have been something like
> "intel->do_blorp_op(params);".  Incidentally, I think we would get a lot
> of benefits out of making brw_context, intel_context, and gl_context a
> class hierarchy; for example, macros like OUT_BATCH() would become
> ordinary member functions, and we wouldn't have to do anywhere near as
> many explicit conversions between the three context types.  Also, the
> virtual function tables that we currently code up explicitly would be
> automatically created by the compiler, so we would have much less risk
> of problems like forgetting to dispatch through the virtual function
> table, forgetting to initialize a function pointer properly, or
> initializing a function pointer at the wrong time.  Of course, this
> would have been *way* too big a change to try to slip into this patch
> series.  By orders of magnitude.

Yeah...way too much churn.  I'd rather leave it the way it is for now. 
C++ in Mesa until now has been restricted to compiler related code, and 
I know many people would like to keep it that way.  More pragmatically, 
in our driver, we use several C99 features (which are really convenient) 
that g++ can't compile.

> - Make a base class to represent the Blorp engine, with a derived class
> for Gen6 and a derived class for Gen7.  The appropriate object would be
> created at context creation time.  Then, instead of calling
> "params.exec(intel);", you would do "brw->blorp->exec(params);".  I
> largely dismissed this option because I had already done so much
> refactoring of the HiZ code that it seemed like it was time to settle
> down and actually implement some new functionality.  But I would still
> be open to it if people like the idea.

Since you've already made it C++, that might not be a bad idea.  I'm not 
sure if it's worth spending time on, though.  My ultimate concern is to 
keep the code readable and maintainable, so if it helps, then sure, why 
not.  Otherwise, no real need...