----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3662/#review8819 -----------------------------------------------------------
Ship it! A few minor issues, mostly related to pre-existing code. If you feel like addressing them while you're touching this code that would be great. Otherwise it looks good to me. src/sim/syscall_debug_macros.hh (line 5) <http://reviews.gem5.org/r/3662/#comment7602> This is not the correct license. See the code in gpu-compute for the license we should be using. src/sim/syscall_desc.hh (line 66) <http://reviews.gem5.org/r/3662/#comment7603> (void) is not necessary in c++. also this is not gem5 convention. src/sim/syscall_desc.hh (line 91) <http://reviews.gem5.org/r/3662/#comment7604> Can we use std::string here? Might as well change it now if you're touching this code anyhow. src/sim/syscall_desc.hh (line 94) <http://reviews.gem5.org/r/3662/#comment7605> Not a major issue, and I know you didn't add this either, but I think it'd be better to have a more explicit name for this: SysCallExecutor or something like that. Really anything more descriptive that FuncPtr would be ok. - Tony Gutierrez On Oct. 11, 2016, 2:06 p.m., Brandon Potter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3662/ > ----------------------------------------------------------- > > (Updated Oct. 11, 2016, 2:06 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11674:30214967f119 > --------------------------- > syscall_emul: move SyscallDesc into its own .hh and .cc files > > The class was crammed into syscall_emul.hh which has tons of forward > declarations and template definitions. To clean it up a bit, moved the > class into separate files and commented the class with doxygen style > comments. Also, provided some encapsulation by adding some accessors and > a mutator. > > The syscallreturn.hh file was renamed syscall_return.hh to make it consistent > with other similarly named files in the src/sim directory. > > The DPRINTF_SYSCALL macro was moved into its own header file with the > include the Base and Verbose flags as well. > > > Diffs > ----- > > src/sim/syscall_desc.cc PRE-CREATION > src/sim/syscall_emul.hh 220fa4099b9a91526b8a1828f27cf1a9f3c15837 > src/sim/syscall_emul.cc 220fa4099b9a91526b8a1828f27cf1a9f3c15837 > src/sim/syscallreturn.hh 220fa4099b9a91526b8a1828f27cf1a9f3c15837 > src/arch/alpha/linux/process.cc 220fa4099b9a91526b8a1828f27cf1a9f3c15837 > src/arch/arm/freebsd/process.cc 220fa4099b9a91526b8a1828f27cf1a9f3c15837 > src/arch/arm/linux/process.cc 220fa4099b9a91526b8a1828f27cf1a9f3c15837 > src/arch/mips/linux/process.cc 220fa4099b9a91526b8a1828f27cf1a9f3c15837 > src/arch/power/linux/process.cc 220fa4099b9a91526b8a1828f27cf1a9f3c15837 > src/arch/sparc/linux/process.cc 220fa4099b9a91526b8a1828f27cf1a9f3c15837 > src/arch/sparc/linux/syscalls.cc 220fa4099b9a91526b8a1828f27cf1a9f3c15837 > src/arch/sparc/solaris/process.cc 220fa4099b9a91526b8a1828f27cf1a9f3c15837 > src/arch/x86/linux/process.cc 220fa4099b9a91526b8a1828f27cf1a9f3c15837 > src/arch/x86/process.cc 220fa4099b9a91526b8a1828f27cf1a9f3c15837 > src/kern/tru64/tru64.hh 220fa4099b9a91526b8a1828f27cf1a9f3c15837 > src/sim/SConscript 220fa4099b9a91526b8a1828f27cf1a9f3c15837 > src/sim/process.hh 220fa4099b9a91526b8a1828f27cf1a9f3c15837 > src/sim/process.cc 220fa4099b9a91526b8a1828f27cf1a9f3c15837 > src/sim/syscall_debug_macros.hh PRE-CREATION > src/sim/syscall_desc.hh PRE-CREATION > > Diff: http://reviews.gem5.org/r/3662/diff/ > > > Testing > ------- > > > Thanks, > > Brandon Potter > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
