> On Sept. 14, 2012, 7:54 a.m., Steve Reinhardt wrote: > > src/SConscript, line 945 > > <http://reviews.gem5.org/r/1409/diff/1/?file=29613#file29613line945> > > > > this is pretty minor, but the formatting on this line is pretty > > awkward... why not wrap on a comma in the outer list (which I think could > > keep this to two lines) rather than bunching everything at the end across > > four? > > > > or you could do it in two steps, assign the flag list to a variable > > then do: > > dict(zip(target_types, linker_flags) > > > > Andreas Hansson wrote: > It wasn't the pretties way for sure. I added a perf_ldflags variable on > the line before and thus got it all in two lines now. > > Nathan Binkert wrote: > I'm bikeshedding here, but I personally would much rather see the dict > written out. I'm not convinced that you've saved more than a dozen > characters of typing really, and it certainly is more prone to errors. > > Andreas Hansson wrote: > Can also go with: > > ldflags = dict(zip(target_types, [[], [], [], ['-pg'], > ['-Wl,--no-as-needed', '-lprofiler', > '-Wl,--as-needed']])) > > Nathan Binkert wrote: > No, I meant: > ldflags = { 'debug' : [], 'opt' : [], 'fast' : [], 'prof' : ... > > I think the dict(zip()) thing is error prone. > > Andreas Hansson wrote: > Don't you think that retyping the target_types in all these locations is > equally bad if not worse in terms of being error prone? > > Steve Reinhardt wrote: > I didn't mean to open up this can of worms, but I agree with Nate, the > explicit dict, while more verbose, is a lot clearer and more maintainable. > > The same goes for obj2target further down. > > > Steve Reinhardt wrote: > The key thing is that if you extend, reorder, or otherwise change > target_types, the zip method can potentially fail silently (e.g., by mapping > the wrong flags to the wrong target in a way that would be hard for someone > other than you to track down), while the dict method will either continue to > work correctly or give you an error (e.g., undefined key).
Reshuffled the previous patch as well to use the explicit dict instead of python magic. Hopefully the can of worms is now officially closed :) - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1409/#review3476 ----------------------------------------------------------- On Sept. 14, 2012, 9 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1409/ > ----------------------------------------------------------- > > (Updated Sept. 14, 2012, 9 a.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 9226:294dbafc3644 > --------------------------- > scons: Add a target for google-perftools profiling > > This patch adds a new target called 'perf' that facilitates profiling > using google perftools rather than gprof. The perftools CPU profiler > offers plenty useful information in addition to gprof, and the latter > is kept mostly to offer profiling also on non-Linux hosts. > > > Diffs > ----- > > src/SConscript be1c1059438b > > Diff: http://reviews.gem5.org/r/1409/diff/ > > > Testing > ------- > > Compiled and ran a few experiments using the perf binary > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
