ldrumm added inline comments.
================
Comment at: scripts/Python/prepare_binding_Python.py:222
+ )
+ logging.info("running swig with: %r", command)
----------------
bryant wrote:
> ldrumm wrote:
> > bryant wrote:
> > > You can reduce diff noise by limiting your changes to removing the %s. So,
> > >
> > > ```python
> > > # Build the SWIG args list
> > > options.swig_executable,
> > > "-c++",
> > > "-shadow",
> > > "-python",
> > > "-threads",
> > > "-I" + os.path.normcase(
> > > os.path.join(options.src_root, "include")),
> > > "-I" + os.path.normcase("./."),
> > > "-D__STDC_LIMIT_MACROS",
> > > "-D__STDC_CONSTANT_MACROS"]
> > > if options.target_platform == "Darwin":
> > > command.append("-D__APPLE__")
> > > if options.generate_dependency_file:
> > > command.extend(["-MMD", " -MF", temp_dep_file_path])
> > > command.extend([
> > > "-outdir", config_build_dir,
> > > "-o", settings.output_file,
> > > settings.input_file
> > > ])
> > > logging.info("running swig with: %s", command)
> > > ```
> > But `logging.info` is not a pretty printer - if the command fails for some
> > reason we need to see why. `repr` allows this, and the diff noise is again
> > minimal because that line is changing anyway and command is now a list, not
> > a string
> Yes, that was a typo. Keep %r too.
Sorry - I just realised you meant the whole block (I only caught on slowly
beyond the logging call).
I don't see a problem with the current implementation - it has some technical
advantages over the existing code:
- The list of args is built as a single expression, which is notionally nicer
than modifying it piecemeal
- The definition of the optional constants upfront is done such that they are
always defined one way or another
- `os.path.normcase(os.path.join(options.src_root, "include"))` is incorrect as
[there can be case-sensitive filesystems on win32](http://www.ext2fsd.com/)
- `os.path.normcase("./."),` doesn't make sense on any OS I’m aware of (though
I need to change this to `os.path.curdir` as it's possible some OS or
filesystem doesn't use `"."`)
Given that I'm fixing at least 4 broken assumptions here, I feel that the
cleanup is worth a little diff noise
https://reviews.llvm.org/D26757
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
