This is an automatically generated e-mail. To reply, visit:

src/sim/fd_array.hh (line 109)

    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)

    Do you need to cast here?

src/sim/fd_array.cc (line 88)

    same here

src/sim/fd_array.cc (line 99)

    same here

src/sim/fd_entry.hh (line 67)

    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)

    Can you rename this to something more descriptive, like HostMappedFDEntry?

src/sim/fd_entry.hh (line 156)

    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)

    Should probably just add your name here, not delete the others.

src/sim/process.cc (line 245)

    Why remove?  Does this not work anymore?

src/sim/syscall_emul.hh (line 631)

    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

Reply via email to