> On 2011-10-21 13:10:26, Gabe Black wrote: > > src/mem/page_table.cc, line 137 > > <http://reviews.m5sim.org/r/895/diff/1/?file=15316#file15316line137> > > > > These brackets aren't really needed, although they aren't hurting > > anything.
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. > On 2011-10-21 13:10:26, Gabe Black wrote: > > src/sim/syscall_emul.hh, line 1048 > > <http://reviews.m5sim.org/r/895/diff/1/?file=15317#file15317line1048> > > > > 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. 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 ;-). - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/895/#review1617 ----------------------------------------------------------- On 2011-10-21 07:53:37, Steve Reinhardt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/895/ > ----------------------------------------------------------- > > (Updated 2011-10-21 07:53:37) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > syscall_emul: implement MAP_FIXED option to mmap() > > > Diffs > ----- > > src/arch/alpha/linux/linux.hh c7fec2cb91cb > src/arch/alpha/tru64/tru64.hh c7fec2cb91cb > src/arch/arm/linux/linux.hh c7fec2cb91cb > src/arch/mips/linux/linux.hh c7fec2cb91cb > src/arch/power/linux/linux.hh c7fec2cb91cb > src/arch/sparc/linux/linux.hh c7fec2cb91cb > src/arch/sparc/solaris/solaris.hh c7fec2cb91cb > src/arch/x86/linux/linux.hh c7fec2cb91cb > src/mem/page_table.hh c7fec2cb91cb > src/mem/page_table.cc c7fec2cb91cb > src/sim/syscall_emul.hh c7fec2cb91cb > > Diff: http://reviews.m5sim.org/r/895/diff > > > Testing > ------- > > > Thanks, > > Steve > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
