Ok. I think you should make the change given that the checks around it follow that convention (see libcxx_src_root, libcxx_obj_root, etc).
Once that is fixed, LGTM. Michael On Jul 29, 2013, at 4:05 PM, Sebastian Redl <[email protected]> wrote: > > On 29.07.2013, at 21:58, Michael Gottesman wrote: > >> Hey Sebastian. >> >> Your patch looks fine to me except that you should indent in the inferring >> section so it looks more like this: >> >> if cxx_under_test is None: >> cxx_under_test = getattr(config, 'cxx_under_test', None) >> >> <-- Indent more in starting here >> +if cxx_under_test is None: >> # If no specific cxx_under_test was given, attempt to infer it as >> clang++. >> clangxx = lit.util.which('clang++', config.environment['PATH']) >> if clangxx is not None: >> cxx_under_test = clangxx >> lit.note("inferred cxx_under_test as: %r" % (cxx_under_test,)) >> <-- And stopping here. >> >> Otherwise if cxx_under_test is initially not None, you are checking one more >> time that cxx_under_test is None than you need to. > > That was intentional. I consider less indentation and the readability bonus > from it more important than avoiding one redundant check. > > Of course, if you want it the other way, I can easily change it. > > Sebastian _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
