> 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?
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 ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1409/#review3476 ----------------------------------------------------------- On Sept. 14, 2012, 8:16 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, 8:16 a.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 9226:80a0423ec9e5 > --------------------------- > 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
