src/sim/syscall_emul.hh (Diff revision 1)

isn't this the wrong form with the previous patch?

1. Steve Reinhardt Oct. 21, 2011, 3:07 a.m. (Oct. 21, 2011, 3:07 a.m.)

   You're right, but this one is the earlier patch in my stack, so this line gets fixed in the other patch (though since I used 'postreview -o' that might not show up in the other diff).

This seems mostly reasonable and useful, but the goto really, really needs to go.

src/mem/page_table.cc (Diff revision 1)

These brackets aren't really needed, although they aren't hurting anything.

1. Steve Reinhardt Oct. 21, 2011, 6:50 a.m. (Oct. 21, 2011, 6:50 a.m.)

   Although I see the wiki isn't explicit about it, I've always thought it was part of the style to have braces even around single-statement blocks, as that makes it easier to expand those blocks with additional statements without accidentally messing up your semantics.  I certainly remember having discussions about this way back when and thought that we had settled on that rule.

src/sim/syscall_emul.hh (Diff revision 1)

Long line? It's hard to tell in the browser.

src/sim/syscall_emul.hh (Diff revision 1)

goto! Yikes! This needs to go! It makes the flow of this function hard to follow, and I'm sure it's not doing the compiler any favors.

1. Steve Reinhardt Oct. 21, 2011, 7:09 a.m. (Oct. 21, 2011, 7:09 a.m.)

   Actually I don't think it's hard to follow at all.  Normally we pick the next available address if the user doesn't provide one, and in the somewhat exceptional case that the user provided an address *but* part of the range is already in use *and* the MAP_FIXED flag wasn't specified to force an override of that prior use, then you also fall back on that policy.  Obviously the goto isn't strictly necessary, but IMO rewriting to avoid the goto does not make the logic more apparent or easier to follow (if anything, the opposite).
   
   You are right that goto can have a negative impact on compiler optimization, but if you're spending a measurable fraction of your runtime calling mmap() then you have bigger problems than suboptimal simulation speed.
   
   That said, I was feeling somewhat cantankerous and contrarian when I wrote this, and decided to leave it as is just to see if anyone noticed or cared.  (Not sure whether Ali didn't notice or agreed with me on the merits.)  I'm feeling more mellow today, certainly not cantankerous enough to sustain an argument over leaving this in, so I'll back off and write the IMO slightly less understandable  yet politically correct goto-less version ;-).