On Tue, Apr 14, 2015 at 7:26 PM, Nilay Vaish <[email protected]> wrote:
> On Tue, 14 Apr 2015, Joel Hestness wrote: > > Hi Nilay, >> >> In this case, I opened an issue on this patch that I wanted to see fixed, >> and I even went so far as to propose what I still view as a more >> appropriate solution. My request wasn't even addressed. This is very >> different than no one commenting on a review request and you checking it >> in. >> >> I read nearly all Ruby review requests. I rarely comment, either because >> I'm fine with the concept of the patch, or I don't have the expertise to >> quickly understand the changes to the code. I can offer to increase my >> vocalization in reviews, but we have to make sure we use appropriate >> process when deciding to check in a patch. If an issue has been opened, it >> needs to be addressed. >> >> I hope we all agree that executive decisions can create contention and >> that >> we should avoid them. >> > > I am guessing the author decided not to take any action. I could have > done what you suggested, but I did not feel the requirement. If the author decides not to take action, it doesn't mean that the committer can drop the responsibility of resolving review issues. There are two strong reasons not to use local pointers. First, you don't >> need a local pointer if there is a global one (unless using the global one >> results in a major performance penalty, which is unlikely here). It >> results >> in unnecessary object instance bloat. Second, it is inconsistent with >> other >> uses of the RubySystem, which almost exclusively use g_system_ptr. There >> are 11 existing Ruby system classes that use g_system_ptr to access the >> RubySystem, and basically all generated controllers (including the >> AbstractControllers!) do also. >> >> The only way I can be assuaged is to hear a clearer explanation of why the >> AbstractControllers *should* have their own pointers in this patch. At >> this >> point, I see no reason. >> >> > If you feel the need, just go ahead and revert the patch and / or commit > one that uses the global pointer. I am not opposed to using the global > pointer, but most likely would not do so myself. Making proposed changes is the responsibility of either the author or the committer, not the reviewers. That said, here is my originally proposed solution: http://reviews.gem5.org/r/2738/ Joel -- Joel Hestness PhD Candidate, Computer Architecture Dept. of Computer Science, University of Wisconsin - Madison http://pages.cs.wisc.edu/~hestness/ _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
