Quoting Steve Reinhardt <ste...@gmail.com>:
On Sun, Oct 24, 2010 at 10:05 PM, Gabe Black <gbl...@eecs.umich.edu> wrote:
On 2010-10-22 10:30:06, Steve Reinhardt wrote:
> src/arch/alpha/ev5.cc, line 63
> <http://reviews.m5sim.org/r/255/diff/11/?file=4275#file4275line63>
>
> Why not just redefine setPC() to do this PCState update,
then just delete the setNextPC() line? In general I feel like a
lot of the syntax changes are bigger than necessary. I'm fine
with renaming methods for clarity (e.g., readPC() -> instAddr())
but taking simple functions/assignments and replacing them with
multi-stmt sequences seems like a step backwards.
This is already very close to that. The only difference is that the
new version was split into two lines to keep it from getting
unmanageably long. I'm not sure if g++ would figure out that it
needed to build a PCState object it could attach a const reference
to if the function was called with just an address. There's a
constructor to create a PCState from an address which you see used
here, I just don't know if the language would make that logical leap.
I was thinking more of the idea (that I think Nate mentioned) of
having an overloaded call that just took an Addr as a new PC and did
all the other necessary stuff internally. Whether you want to have
this be an overloaded flavor of pcState() with a different arg type or
give it a different name like setPC() is an open question.
It turns out if you pass it an Addr and it expects a PCState and
there's an appropriate constructor, it just does the conversion for
you. That means you can get what you're talking about just by calling
the existing function differently, a fact I've taken advantage of in
my local version of the patch.
> one more example: why not just delete the last two lines
here, or replace them all with:
> tc->setPC(objFile->entryPoint());
>
We're moving away from setXXX, I thought. Also the fewer methods
that need to be plumbed all over the place the better. tc->pcState
is basically already what you're talking about turning tc->setPC
into, I think. The difference is that the new name is more
appropriate, and it takes a PC state object instead of an address.
Yea, we're moving away from setXX/getXX as accessors for a field named
XX. However in this case, it's not really just an accessor as it's
doing all the side effect stuff of setting NPC etc. I'm not saying
it's the best name, but it does have the advantage of minimizing the
amount of change to legacy code.
My philosophy (which I just made up) is the only distinguishing
characteristic of legacy code is that it's old. It can be painful to
change, but by not changing it you make it older, and even more
painful to change. Then you end up with X86. I'm not going to go
changing things just because I'm bored, but if changing legacy code
makes it better that's what I want to do.
Gabe
_______________________________________________
m5-dev mailing list
m5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/m5-dev
