[Xorp-hackers] [PATCH 1/3] SConstruct fixes

[Ben Greear]

On 09/28/2011 05:39 AM, Igor Maravić wrote:
> Yes that check will be false when optimize is set to override.
>
> override without options CFLAGS and CXXFLAGS is same as option none.
>
> Without my patch, there is no point for the line that I marked with
> <===, because that flag will never be appended to CFLAGS/CXXFLAGS in
> if statement that I proposed to change.
>
> bigodict = { 'no': '-O0',<===
>                   # 'minimal' denotes only those optimizations
>                   # necessary to force gcc to perform the tree_sink
>                   # pass, to elide most STL template instantiations.
>                   'minimal': "-O1 -fno-defer-pop -fno-delayed-branch \
> -fno-guess-branch-probability -fno-cprop-registers -fno-if-conversion \
> -fno-if-conversion2 -fno-tree-ccp -fno-tree-dce -fno-tree-dominator-opts \
> -fno-tree-dse -fno-tree-ter -fno-tree-lrs -fno-tree-sra \
> -fno-tree-copyrename -fno-tree-fre -fno-tree-ch -fno-unit-at-a-time \
> -fno-merge-constants",
>                   'yes': '-O1',
>                   'full': '-O2',
>                   'highest': '-O3',
>                   'size': '-Os' }
>
> Also if someone compiles with the command
>
> scons optimize=override CFLAGS=-01 CXXFLAGS=-O0
>
> there is no need to to append something to CFLAGS/CXXFLAGS, when we
> want to override there behavior.
> I marked the lines with<==, and that is the if statement that I want to change.
>
> if not env['optimize'] == 'no':
>          env.AppendUnique(CFLAGS = [ bigoflag ])<==
>          env.AppendUnique(CXXFLAGS = [ bigoflag ])<==
>
> That's why I think that my patch should be applied.

Look at the entire section of existing code:

If optimize == overide, then the entire branch
is skipped:

# If the user didn't override our default optimization, then
# sanitize user's CFLAGS/CXXFLAGS to not contain optimization options,
# and map to an appropriate GCC flag.
if not env['optimize'] == 'override':

     # Never get here if optimize == override.

     env.Replace( CFLAGS = filter(lambda s: not s.startswith('-O'),
                                  Split(env['CFLAGS'])))
     env.Replace( CXXFLAGS = filter(lambda s: not s.startswith('-O'),
                                    Split(env['CXXFLAGS'])))
     bigodict = { 'no': '-O0',
                  # 'minimal' denotes only those optimizations
                  # necessary to force gcc to perform the tree_sink
                  # pass, to elide most STL template instantiations.
                  'minimal': "-O1 -fno-defer-pop -fno-delayed-branch \
     			-fno-guess-branch-probability -fno-cprop-registers -fno-if-conversion \
     			-fno-if-conversion2 -fno-tree-ccp -fno-tree-dce -fno-tree-dominator-opts \
     			-fno-tree-dse -fno-tree-ter -fno-tree-lrs -fno-tree-sra \
     			-fno-tree-copyrename -fno-tree-fre -fno-tree-ch -fno-unit-at-a-time \
     			-fno-merge-constants",
                  'yes': '-O1',
                  'full': '-O2',
                  'highest': '-O3',
                  'size': '-Os' }
     bigoflag = bigodict[env['optimize']]
     if not env['optimize'] == 'no':
         env.AppendUnique(CFLAGS = [ bigoflag ])
         env.AppendUnique(CXXFLAGS = [ bigoflag ])



I'm not arguing that the current code is right, but just that your
patch is not much better.

I'll post my proposed fix for this later today...

Thanks,
Ben


-- 
Ben Greear <greearb at candelatech.com>
Candela Technologies Inc  http://www.candelatech.com