[llvm-commits] Tblgen refactoring

[Jakob Stoklund Olesen]

Looks good. 

/jakob

On Jan 1, 2013, at 1:34, Sean Silva <silvas at purdue.edu> wrote:

> Jakob, could you take a look at this?
> 
> -- Sean Silva
> 
> On Tue, Jan 1, 2013 at 2:06 AM, Malul, Elior <elior.malul at intel.com> wrote:
>> Attached is a revised patch.
>> Thx, Elior
>> 
>> -----Original Message-----
>> From: Sean Silva [mailto:silvas at purdue.edu]
>> Sent: Tuesday, January 01, 2013 02:31
>> To: Malul, Elior
>> Cc: llvm-commits at cs.uiuc.edu
>> Subject: Re: [llvm-commits] Tblgen refactoring
>> 
>> Please submit a revised patch for review.
>> 
>> -- Sean Silva
>> 
>> On Mon, Dec 31, 2012 at 6:20 AM, Malul, Elior <elior.malul at intel.com> wrote:
>>> Hi Sean.
>>> As you mentioned, the main idea behind the patch is to eliminate the (what seems to me as) excessive overloading of 'subClassOf' for every derived type.
>>> Instead, I have changed it so there will be only one virtual method with the prototype 'virtual void baseClassOf(const RecTy*)'.
>>> Then, the dynamic type to the parameter is done (when needed) by querying the 'getRecTyKind()' method.
>>> It does add some branches to the code, but I think all together, it conforms better to the DRY principle.
>>> 
>>> Thx for your comment. See rest of my comment in the text below:
>>> 
>>> -----Original Message-----
>>> From: Sean Silva [mailto:silvas at purdue.edu]
>>> Sent: Monday, December 31, 2012 13:09
>>> To: Malul, Elior
>>> Cc: llvm-commits at cs.uiuc.edu
>>> Subject: Re: [llvm-commits] Tblgen refactoring
>>> 
>>> This is something that has been bugging me too!
>>> 
>>> Maybe you could explain a bit better the idea behind the patch a bit more? I think that it's the right direction. My understanding is that what you are doing is removing all the different overloads and having a single baseClassOf which dynamically looks at the records it is interested in? If that's correct then that sounds good to me.
>>> 
>>> A few comments:
>>> 
>>> -//===----------------------------------------------------------------------===//
>>> -//    std::string wrapper for DenseMap purposes
>>> -//===----------------------------------------------------------------
>>> ------===//
>>> -
>>> 
>>> --Elior:
>>> OK, I will separate for a different patch.
>>> 
>>> This is unrelated to the current patch. I think it's a good change, but please separate it out into a different patch.
>>> 
>>> +//
>>> +//RecTy
>>> +//
>>> +
>>> 
>>> --Elior:
>>> Same goes here.
>>> 
>>> Same for these "headers" too. I think they are a worthwhile change, but keep them in a separate patch.
>>> 
>>> -    return true;
>>> +   return true;
>>> 
>>> What's the point of dedenting this by 1 space?
>>> 
>>> --Elior:
>>> Accidental space deletion.
>>> 
>>> -- Sean Silva
>>> 
>>> On Mon, Dec 31, 2012 at 2:25 AM, Malul, Elior <elior.malul at intel.com> wrote:
>>>> Hi all, attached is a small patch that I think makes tblgen somewhat
>>>> more maintainable.
>>>> 
>>>> 
>>>> 
>>>> The way code is at the moment, if one want's to add a new tblgen type
>>>> (that is to subclass 'RecTy'),
>>>> 
>>>> Not only he/she compelled to add implementation to all variants of
>>>> 'baseClassOf',
>>>> 
>>>> one also needs to add a new 'baseClassOf' method for the newly added
>>>> types to *all* the types in the hierarchy.
>>>> 
>>>> The attached patch tries to deal with that phenomena.
>>>> 
>>>> 
>>>> 
>>>> I will appreciate if those who care about tblgen will review it.
>>>> 
>>>> Thx, Elior
>>>> 
>>>> ---------------------------------------------------------------------
>>>> Intel Israel (74) Limited
>>>> 
>>>> This e-mail and any attachments may contain confidential material for
>>>> the sole use of the intended recipient(s). Any review or distribution
>>>> by others is strictly prohibited. If you are not the intended
>>>> recipient, please contact the sender and delete all copies.
>>>> 
>>>> 
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>> ---------------------------------------------------------------------
>>> Intel Israel (74) Limited
>>> 
>>> This e-mail and any attachments may contain confidential material for
>>> the sole use of the intended recipient(s). Any review or distribution
>>> by others is strictly prohibited. If you are not the intended
>>> recipient, please contact the sender and delete all copies.
>> ---------------------------------------------------------------------
>> Intel Israel (74) Limited
>> 
>> This e-mail and any attachments may contain confidential material for
>> the sole use of the intended recipient(s). Any review or distribution
>> by others is strictly prohibited. If you are not the intended
>> recipient, please contact the sender and delete all copies.