syscall_emul: [patch 2/22] move SyscallDesc into its own .hh and .cc
Review Request #3662 - Created Oct. 11, 2016 and submitted
Information | |
---|---|
Brandon Potter | |
gem5 | |
default | |
Reviewers | |
Default | |
Changeset 11691:111bd8a24517 --------------------------- syscall_emul: [patch 2/22] move SyscallDesc into its own .hh and .cc 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.
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+227 -98) |
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 (Diff revision 2) -
This is not the correct license. See the code in gpu-compute for the license we should be using.
-
src/sim/syscall_desc.hh (Diff revision 2) -
(void) is not necessary in c++. also this is not gem5 convention.
-
src/sim/syscall_desc.hh (Diff revision 2) -
Can we use std::string here? Might as well change it now if you're touching this code anyhow.
-
src/sim/syscall_desc.hh (Diff revision 2) -
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.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+248 -98) |
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 4 (+248 -98) |
Ship It!
Ship It!
Ship It!
Ship It!
-
src/sim/syscall_desc.hh (Diff revision 4) -
my preference would be to keep this as a one-liner as in the original code. I don't think expanding it makes it more readable. If anything, it makes the code seem more significant than it really is.
-
src/sim/syscall_desc.hh (Diff revision 4) -
if you're going to create methods, might as well put the semantics here too. i.e., no need for separate warned() and setWarned() methods, just do something like:
bool checkWarned() { bool was_warned = _warned; _warned = true; return was_warned; }
and then the code in ignoreFunc() could be simplified a bit. I might even go further and, instead of checkWarned() like above, have a function like this:
bool needWarning() { bool suppress_warning = warnOnce() && !_warned; _warned = true; return !suppress_warning; }
and then you could rewrite ignoreFunc() along the lines of:
if (desc->needWarning()) { warn("ignoring syscall %s(...)%s", desc->name(), desc->warnOnce() ? "\n (further warnings will be suppressed)" : ""); }
-
src/sim/syscall_desc.hh (Diff revision 4) -
again, this could be a one-liner.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+299 -105) |
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+299 -105) |
Ship It!