Re: [gem5-dev] Review Request 3674: syscall_emul: [patch 8/22] refactor process class

2016-10-20 Thread Brandon Potter


> On Oct. 19, 2016, 6:23 p.m., Steve Reinhardt wrote:
> > src/sim/aux_vector.cc, line 42
> > 
> >
> > why do we need to include process.hh here?

"process.hh" is included because it defines the namespace that it needs for 
"THE_ISA::". The isa header doesn't actually declare the namespace; it typedefs 
"THE_ISA" into an architecture specific version.


- Brandon


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3674/#review8933
---


On Oct. 17, 2016, 3:23 p.m., Brandon Potter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3674/
> ---
> 
> (Updated Oct. 17, 2016, 3:23 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11696:c48d378a972a
> ---
> syscall_emul: [patch 8/22] refactor process class
> 
> Moves aux_vector into its own .hh and .cc files just to get it out of the
> already crowded Process files. Arguably, it could stay there, but it's
> probably better just to move it and give it files.
> 
> The changeset looks ugly around the Process header file, but the goal here is
> to move methods and members around so that they're not defined randomly
> throughout the entire header file. I expect this is likely one of the reasons
> why I several unused variables related to this class. So, the methods are
> declared first followed by members. I've tried to aggregate them together
> so that similar entries reside near one another.
> 
> There are other changes coming to this code so this is by no means the
> final product.
> 
> 
> Diffs
> -
> 
>   src/sim/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/sim/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/sim/aux_vector.cc PRE-CREATION 
>   src/sim/aux_vector.hh PRE-CREATION 
>   src/sim/SConscript 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/arch/x86/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/arch/x86/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/arch/sparc/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/arch/power/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/arch/mips/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/arch/alpha/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/arch/arm/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
> 
> Diff: http://reviews.gem5.org/r/3674/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Brandon Potter
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] Review Request 3674: syscall_emul: [patch 8/22] refactor process class

2016-10-19 Thread Steve Reinhardt

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3674/#review8933
---



src/sim/aux_vector.hh (line 2)


In general, if you're creating a new file that contains pre-existing code 
from another file, the new file should inherit the original file's copyright 
and author list. Those apply to the code itself, and not the file as an entity. 
If you make changes or additions, you can then add the AMD 2016 copyright and 
your name to the author list. What you have here though makes it look like you 
wrote this code yourself from scratch in 2016, which is not right. I believe 
this applies in some of your other patches as well, though I neglected to flag 
it there.



src/sim/aux_vector.cc (line 42)


why do we need to include process.hh here?


- Steve Reinhardt


On Oct. 17, 2016, 8:23 a.m., Brandon Potter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3674/
> ---
> 
> (Updated Oct. 17, 2016, 8:23 a.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11696:c48d378a972a
> ---
> syscall_emul: [patch 8/22] refactor process class
> 
> Moves aux_vector into its own .hh and .cc files just to get it out of the
> already crowded Process files. Arguably, it could stay there, but it's
> probably better just to move it and give it files.
> 
> The changeset looks ugly around the Process header file, but the goal here is
> to move methods and members around so that they're not defined randomly
> throughout the entire header file. I expect this is likely one of the reasons
> why I several unused variables related to this class. So, the methods are
> declared first followed by members. I've tried to aggregate them together
> so that similar entries reside near one another.
> 
> There are other changes coming to this code so this is by no means the
> final product.
> 
> 
> Diffs
> -
> 
>   src/sim/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/sim/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/sim/aux_vector.cc PRE-CREATION 
>   src/sim/aux_vector.hh PRE-CREATION 
>   src/sim/SConscript 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/arch/x86/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/arch/x86/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/arch/sparc/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/arch/power/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/arch/mips/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/arch/alpha/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/arch/arm/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
> 
> Diff: http://reviews.gem5.org/r/3674/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Brandon Potter
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] Review Request 3674: syscall_emul: [patch 8/22] refactor process class

2016-10-17 Thread Michael LeBeane

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3674/#review8879
---

Ship it!


Ship It!

- Michael LeBeane


On Oct. 17, 2016, 3:23 p.m., Brandon Potter wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3674/
> ---
> 
> (Updated Oct. 17, 2016, 3:23 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11696:c48d378a972a
> ---
> syscall_emul: [patch 8/22] refactor process class
> 
> Moves aux_vector into its own .hh and .cc files just to get it out of the
> already crowded Process files. Arguably, it could stay there, but it's
> probably better just to move it and give it files.
> 
> The changeset looks ugly around the Process header file, but the goal here is
> to move methods and members around so that they're not defined randomly
> throughout the entire header file. I expect this is likely one of the reasons
> why I several unused variables related to this class. So, the methods are
> declared first followed by members. I've tried to aggregate them together
> so that similar entries reside near one another.
> 
> There are other changes coming to this code so this is by no means the
> final product.
> 
> 
> Diffs
> -
> 
>   src/sim/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/sim/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/sim/aux_vector.cc PRE-CREATION 
>   src/sim/aux_vector.hh PRE-CREATION 
>   src/sim/SConscript 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/arch/x86/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/arch/x86/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/arch/sparc/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/arch/power/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/arch/mips/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/arch/alpha/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
>   src/arch/arm/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
> 
> Diff: http://reviews.gem5.org/r/3674/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Brandon Potter
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev


[gem5-dev] Review Request 3674: syscall_emul: [patch 8/22] refactor process class

2016-10-17 Thread Brandon Potter

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3674/
---

Review request for Default.


Repository: gem5


Description
---

Changeset 11696:c48d378a972a
---
syscall_emul: [patch 8/22] refactor process class

Moves aux_vector into its own .hh and .cc files just to get it out of the
already crowded Process files. Arguably, it could stay there, but it's
probably better just to move it and give it files.

The changeset looks ugly around the Process header file, but the goal here is
to move methods and members around so that they're not defined randomly
throughout the entire header file. I expect this is likely one of the reasons
why I several unused variables related to this class. So, the methods are
declared first followed by members. I've tried to aggregate them together
so that similar entries reside near one another.

There are other changes coming to this code so this is by no means the
final product.


Diffs
-

  src/sim/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
  src/sim/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 
  src/sim/aux_vector.cc PRE-CREATION 
  src/sim/aux_vector.hh PRE-CREATION 
  src/sim/SConscript 4a86763c0b30cccba0f56c7f48637a46a4663b06 
  src/arch/x86/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
  src/arch/x86/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 
  src/arch/sparc/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
  src/arch/power/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
  src/arch/mips/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
  src/arch/alpha/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 
  src/arch/arm/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 

Diff: http://reviews.gem5.org/r/3674/diff/


Testing
---


Thanks,

Brandon Potter

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev