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.
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.
--
Nilay
_______________________________________________
gem5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/gem5-dev
