Hey Chris, The following patch removes all global references to a RecordKeeper instance for the tblgen utility. This effort was motivated by the FIXME remark, in TableGen.cpp. Although a few files were touched, the main change was to Record.h. The patch takes the simple approach of adding a RecordKeeper reference to TGParser, and any needed emitter helper classes. In addition, since some of the classes, and methods in Record reference the previously global RecordKeeper instance, I added another RecordKeeper reference to Record, along with such a typed argument to Record's constructor, and a corresponding factory method to RecordKeeper. Do you want to proceed with this approach, or are there any tweaks you want made? Garrison PS: This patch is standalone with respect to trunk: 121636 -------------- next part -------------- A non-text attachment was scrubbed... Name: RecordKeeper.3.patch Type: application/octet-stream Size: 22641 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20101212/a61b1e79/attachment.obj>
On Dec 12, 2010, at 10:54 AM, Garrison Venn wrote:> > Hey Chris, > > The following patch removes all global references to a RecordKeeper instance for the tblgen > utility. This effort was motivated by the FIXME remark, in TableGen.cpp. Although a few files > were touched, the main change was to Record.h. > > The patch takes the simple approach of adding a RecordKeeper reference to TGParser, and > any needed emitter helper classes. In addition, since some of the classes, and methods in Record > reference the previously global RecordKeeper instance, I added another RecordKeeper > reference to Record, along with such a typed argument to Record's constructor, and a > corresponding factory method to RecordKeeper.This looks like great progress to me, applied in r121659. That said, it is suboptimal for Record to have to have a RecordKeeper& in it. I haven't looked at the code in a long time, is it feasible to detangle that out of record, or is it not worth it? Another random question: why is createRecord better than using "new Record"? If createRecord is better, it would be good to make the Record ctor private so the code doesn't evolve into sometimes using one and sometimes using the other. Some minor coding style things: + RecordKeeper &getRecords() const { + return(TrackedRecords); + } + Record *createRecord(const std::string &N, SMLoc loc) { + return(new Record(N, loc, *this)); + } No need for the parens around the return value. + RecordKeeper& getRecords() { + return(Records); + } + + RecordKeeper& getRecords() const { + return(Records); + } Ditto. Also, if you have the later, you don't need the former version of this. Also, please put a space before the & instead of after it, giving: + RecordKeeper &getRecords() const { + return Records; + } + /// Tracked Records + RecordKeeper& Records; Same thing with &/space spacement. Otherwise the patch looks great, thanks for working on this Garrison! -Chris
Hi Chris, Thanks for the review! I believe I caught most of the syntax style issues with the attached patch. It only contains these style changes. On Dec 12, 2010, at 19:30, Chris Lattner wrote:> > On Dec 12, 2010, at 10:54 AM, Garrison Venn wrote: > >> >> Hey Chris, >> >> The following patch removes all global references to a RecordKeeper instance for the tblgen >> utility. > > This looks like great progress to me, applied in r121659. > > That said, it is suboptimal for Record to have to have a RecordKeeper& in it. I haven't looked at the code in a long time, is it feasible to detangle that out of record, or is it not worth it?I'll see if I can come up with another approach. This internal reference was motivated by the Record::setName(...) and UnOpInit::Fold(...) implementations accessing the previous global RecordKeeper instance. We could add a RecordKeeper& argument to setName and OpInit::Fold(...). However as I'm looking at the code, while adding this argument to Fold for TGParser use is not an issue as it has a RecordKeeper instance, the other Record classes have a problem because they lack this type of instance. See for example the implementation of Record.cpp:OpInit::resolveBitReference(...). These methods (such as resolveBitReference(...) and resolveReferences(...) need a context containing a RecordKeeper for their evaluation. A cursory glance for the uses of Record::setName(...) seems to imply that adding a RecordKeeper& argument would not be an issue. I'll keep on looking for other mechanisms.> > Another random question: why is createRecord better than using "new Record"? If createRecord is better, it would be good to make the Record ctor private so the code doesn't evolve into sometimes using one and sometimes using the other.This was another syntactic hack. I personally prefer the factory approach, but I cannot as it stands get rid of the explicit Record constructor because of MultiClass. It has member variable of type Record and constructs this member directly using the Record constructor. It therefore does not need the allocation provided by createRecord. I could push the Record constructor to protected or private and make MultiClass a friend. This is ugly so I think I should drop the factory method in favor of new Record. Like I said the factory method is mainly sugar anyway. I'm assuming your ok with dropping the RecordKeeper::createRecord(...). I'll send that patch tomorrow. Thanks again for taking time to review. Garrison>> Some minor coding style things: > > ...> -Chris >-------------- next part -------------- A non-text attachment was scrubbed... Name: RecordKeeper.4.patch Type: application/octet-stream Size: 4666 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20101212/da76c8f5/attachment.obj>