-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/366/#review632
-----------------------------------------------------------

Ship it!


We've beaten this to death, so I don't care about any of my comments enough to 
hold you up if you don't want to deal with them at all.


SConstruct
<http://reviews.m5sim.org/r/366/#comment847>

    Can you derive from object?  I really hate classic classes as they make 
various things annoying and the cause is not always obvious.



SConstruct
<http://reviews.m5sim.org/r/366/#comment851>

    I hate lines like this.  I'll register my annoyance, but won't continue 
further discussion.



SConstruct
<http://reviews.m5sim.org/r/366/#comment848>

    This is probably pedantic, but these could be @classmethod (and self should 
be cls)



SConstruct
<http://reviews.m5sim.org/r/366/#comment849>

    99 seems like a lot and goes against the idea of being concise.  I don't 
care much though.
    
    Would anyone object to [%-8s] instead?  the right justified seems odd.



SConstruct
<http://reviews.m5sim.org/r/366/#comment850>

    out of curiosity, can you do the following instead?
    main['CCCOMSTR'] = TRANSFORM("CC")
    
    Certainly, I am pretty sure that the parameter to MakeAction/Action could 
be the callable and not the string.  (String substitution just slows down SCons)


- Nathan


On 2011-01-06 11:15:30, Steve Reinhardt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.m5sim.org/r/366/
> -----------------------------------------------------------
> 
> (Updated 2011-01-06 11:15:30)
> 
> 
> Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and 
> Nathan Binkert.
> 
> 
> Summary
> -------
> 
> scons: show sources and targets when building.
> 
> I like the brevity of Ali's recent change, but the ambiguity of
> sometimes showing the source and sometimes the target is a little
> confusing.  This patch makes scons typically list all sources and
> all targets for each action, with the common path prefix factored
> out for brevity.  It's a little more verbose now but also more
> informative.
> 
> 
> Diffs
> -----
> 
>   SConstruct 9f9e10967912 
>   src/SConscript 9f9e10967912 
>   src/arch/SConscript 9f9e10967912 
>   src/arch/isa_parser.py 9f9e10967912 
> 
> Diff: http://reviews.m5sim.org/r/366/diff
> 
> 
> Testing
> -------
> 
> quick regressions pass
> 
> 
> Screenshots
> -----------
> 
> sample colorized output
>   http://reviews.m5sim.org/r/366/s/1/
> 
> 
> Thanks,
> 
> Steve
> 
>

_______________________________________________
m5-dev mailing list
m5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/m5-dev

Reply via email to