----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3676/#review8881 -----------------------------------------------------------
src/sim/fd_array.hh (line 109) <http://reviews.gem5.org/r/3676/#comment7642> This seems weird. I'm pretty sure the '=' operator has access to all the private members and that this is not needed. src/sim/fd_array.cc (line 75) <http://reviews.gem5.org/r/3676/#comment7639> Do you need to cast here? src/sim/fd_array.cc (line 88) <http://reviews.gem5.org/r/3676/#comment7641> same here src/sim/fd_array.cc (line 99) <http://reviews.gem5.org/r/3676/#comment7640> same here src/sim/fd_entry.hh (line 67) <http://reviews.gem5.org/r/3676/#comment7638> Maybe mark as TODO? Should also put a warn when trying to (un)serialize if you don't think it will work correctly. src/sim/fd_entry.hh (line 128) <http://reviews.gem5.org/r/3676/#comment7636> Can you rename this to something more descriptive, like HostMappedFDEntry? src/sim/fd_entry.hh (line 156) <http://reviews.gem5.org/r/3676/#comment7637> If you move simFD to the base class you can avoid a lot of your downcasts later. I don't think it's a big deal that DeviceFDEntry doesn't use it. src/sim/fd_entry.cc (line 33) <http://reviews.gem5.org/r/3676/#comment7643> Should probably just add your name here, not delete the others. src/sim/process.cc (line 245) <http://reviews.gem5.org/r/3676/#comment7644> Why remove? Does this not work anymore? src/sim/syscall_emul.hh (line 631) <http://reviews.gem5.org/r/3676/#comment7645> These downcasts are messy, see above for suggestion on getting rid of them. - Michael LeBeane On Oct. 17, 2016, 3:41 p.m., Brandon Potter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3676/ > ----------------------------------------------------------- > > (Updated Oct. 17, 2016, 3:41 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11698:502f94aa9638 > --------------------------- > syscall_emul: [patch 10/22] refactor fdentry and add fdarray class > > Several large changes happen in this patch. > > The FDEntry class is rewritten so that file descriptors now correspond to > types: 'Regular' which is normal file-backed file with the file open on the > host machine, 'Pipe' which is a pipe that has been opened on the host machine, > and 'Device' which does not have an open file on the host yet acts as a pseudo > device with which to issue ioctls. Other types which might be added in the > future are directory entries and sockets (off the top of my head). > > The FDArray class was create to hold most of the file descriptor handling > that was stuffed into the Process class. It uses shared pointers and > the std::array type to hold the FDEntries mentioned above. The implementation > could use a review; I feel that there's some room for improvement, but it > seems like a decent first step. > > The changes to these two classes needed to be propagated out to the rest > of the code so there were quite a few changes for that. Also, comments were > added where I thought they were needed to help others and extend our > DOxygen coverage. > > > Diffs > ----- > > src/sim/syscall_emul.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/syscall_emul.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/fd_array.hh PRE-CREATION > src/sim/fd_array.cc PRE-CREATION > src/sim/fd_entry.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/fd_entry.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/SConscript 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/kern/tru64/tru64.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > > Diff: http://reviews.gem5.org/r/3676/diff/ > > > Testing > ------- > > > Thanks, > > Brandon Potter > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
