I spent the day working over the patches, so even though it might be another few days until we send out updated versions, I wanted to mention how I've addressed these issues:
On Mon, Dec 21, 2009 at 10:19 AM, Steve Reinhardt <[email protected]> wrote: > On Mon, Dec 21, 2009 at 9:03 AM, nathan binkert <[email protected]> wrote: >> Overall, things look pretty good. There are more or less four things >> that you've done that need work. >> >> 1) You created SimObjects for all of the ruby objects and in the >> process, you didn't give any help strings to any of the parameters. >> While I understand that this can be tedious, this is one minimal form >> of documentation that does help users. I found the original Ruby documentation strings (turned out they were in comments in defaults.rb and not in cfg.rb where I thought they'd be) and copied them over, so at least we're not losing any ground there. I'm not the person to document the previously undocumented params though. >> 2) You removed Garnet from the compile. Do we want to keep garnet? >> If so, this is guaranteed to lead to bit rot. We definitely want to keep it and will be porting it over. It's a fairly substantial but separable piece though so we'll do it in a separate patch. >> 4) You've added #includes in places, but have not maintained sorted >> order. It's a lot easier to visually scan #includes if they're all >> sorted. This lets you easily find duplicates and quickly figure out >> if you need to add something. (There's something in the coding style >> document about this.) I started to do this, but the original includes were not sorted, so it causes cascading problems through the whole patch set when you sort everything the first time you add an include. I can see where that could complicate integrating Derek's changes. I think this is better done in a style-only pass at the end... we'll need at least one of those to fix indentation etc. anyway. Steve _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
