[llvm-commits] [llvm] r69810 - in /llvm/trunk: docs/TableGenFundamentals.html test/TableGen/MultiClassInherit.td utils/TableGen/TGParser.cpp utils/TableGen/TGParser.h

[David Greene]

On Thursday 30 April 2009 13:37, Bob Wilson wrote:
> Hi David,
>
> I'm looking forward to using this new feature, so I volunteered to
> review it.  I found a few very superficial problems, which I have gone
> ahead and fixed.  Otherwise, it looks great, especially from the
> perspective of the syntax and behavior of the new multiclass
> inheritance feature -- the internal details of TableGen are still a
> bit murky to me, but your change looks OK as far as I understand it.

It's a bit murky to me too.  :)

Thanks for reviewing this.  Can you post your fixes?

> One thing I noticed is that the new AddSubMultiClass method has a
> formal parameter that is always the same as the
> TGParser::CurMultiClass instance variable.  I renamed the formal
> parameter so it doesn't shadow the instance variable, but another
> option would be to remove the formal parameter altogether.  It seems
> like there are several other methods in this class with the same
> issue, so I'm guessing that this is intentional.  There is certainly
> something to be said for being explicit with the parameter -- getting
> rid of the instance variable would be nicer in some ways, but I have
> no idea how difficult that would be.  Anyway, I think it's OK the way
> it is but thought I would raise the issue in case you or someone else
> wants to make further changes.

I intentially passed it explicitly so that this patch wouldn't explicitly rely 
on a class-scope variable.  I agree with renaming the formal parameter so it 
doesn't shadow the instance variable.

As I understand things, the multiclass implementation is a bit of a black 
sheep, relying on the CurMultiClass variable both as a state specifier (am
I parsing a multiclass or not?) and a conveyor of information (what multiclass 
am I looking at?).  The consequence is that recursion becomes difficult and 
there are various special cases throughout the code (see ParseDef, for 
example).  I would have preferred to somehow unify the class and multiclass 
inheritance mechanisms because they're very simimlar, but the global state 
represented by CurMultiClass prevents that.

Long term, we should see if we can clean this up.

                                   -Dave