[PATCH] D9600: Add scan-build python implementation
Laszlo Nagy via cfe-commits
cfe-commits at lists.llvm.org
Fri Nov 20 08:42:42 PST 2015
rizsotto.mailinglist marked 13 inline comments as done.
rizsotto.mailinglist added a comment.
thanks Devin for your comments. made some changes already (will upload it tonight after some tests).
================
Comment at: tools/scan-build-py/CHANGES.txt:1
@@ +1,1 @@
+v<version>, <date> -- Initial release.
----------------
dcoughlin wrote:
> Is this one needed too?
in order to make this code a standalone python tool tool, we need this file. (see llvm/utils/lit directory for example.)
================
Comment at: tools/scan-build-py/MANIFEST.in:1
@@ +1,2 @@
+include README.md
+include *.txt
----------------
dcoughlin wrote:
> How about this one? Is it needed in clang trunk?
in order to make this code a standalone python tool tool, we need this file. (see llvm/utils/lit directory for example.)
================
Comment at: tools/scan-build-py/libear/__init__.py:1
@@ +1,2 @@
+# -*- coding: utf-8 -*-
+# The LLVM Compiler Infrastructure
----------------
dcoughlin wrote:
> How does this file fit into the overall build picture? Will this file go away once scan-build-py is built with the common clang cmake?
this is quiet confusing me. previously you were asking make it work without installation. this file makes it possible to compile the `ear` library compiled before the build runs to use as preloaded library. the thing which is not needed is the CMakefile actually.
================
Comment at: tools/scan-build-py/libear/__init__.py:99
@@ +98,3 @@
+ def dl_libraries(self):
+ pass
+
----------------
dcoughlin wrote:
> I gather the intent is that subclasses will override to provide their own versions of these methods? If so, these methods need to be documented so that people adding new support for additional platforms know what they should provide in their subclasses.
>
> If there are reasonable defaults (for example., `[]` for `dl_libraries`), those should be returned here rather than `pass`. If not, these should probably raise an exception indicating they must be implemented rather than silently doing nothing.
now rise `NotImplementedError` runtime exception.
================
Comment at: tools/scan-build-py/libear/__init__.py:166
@@ +165,3 @@
+ self.ctx = context
+ self.results = {'APPLE': sys.platform == 'darwin'}
+
----------------
dcoughlin wrote:
> What does this do? Why is it hard-coded?
this is added to mimic `cmake` behaviour. it is used in the `config.h.in` file.
================
Comment at: tools/scan-build-py/libscanbuild/command.py:20
@@ +19,3 @@
+
+def classify_parameters(command):
+ """ Parses the command line arguments of the given invocation. """
----------------
dcoughlin wrote:
> I think it would be good to document the keys and meaning of the returned dictionary. Or perhaps it would be better represented as class?
now documented when create the return value. (creating class would not bring much to the kitchen i think.)
================
Comment at: tools/scan-build-py/libscanbuild/command.py:23
@@ +22,3 @@
+
+ ignored = {
+ '-g': 0,
----------------
dcoughlin wrote:
> I think it would good to document what the value in this mapping means (number of expected parameters). I realize ccc-analyzer in the original scan-build is similarly un-documented, but we should do better here!
>
> Also: should this include all the arguments `IgnoredOptionMap` in ccc-analyzer? It is missing `-u' and adds '-g'. Or are these changes intentional?
`-u` is part of ignored linker flags. (see a few line above)
`-g` is added to mimic the `ccc-analyzer` results.
comment about key, value is added.
================
Comment at: tools/scan-build-py/libscanbuild/driver.py:1
@@ +1,2 @@
+# -*- coding: utf-8 -*-
+# The LLVM Compiler Infrastructure
----------------
dcoughlin wrote:
> Why is this file called "driver"?
any recommendation? it was the only entry point before the interposition was introduced. so it was the driver of the libscanbuild library.
================
Comment at: tools/scan-build-py/libscanbuild/driver.py:34
@@ +33,3 @@
+def main(bin_dir):
+ """ Entry point for 'scan-build'. """
+
----------------
dcoughlin wrote:
> Should this be 'intercept-build'?
can be anything, but would make it rhyme with the module name...
================
Comment at: tools/scan-build-py/libscanbuild/driver.py:67
@@ +66,3 @@
+ except Exception:
+ logging.exception("Something unexpected had happened.")
+ return 127
----------------
dcoughlin wrote:
> I think this error message can be improved. Perhaps "Unexpected error running intercept-build"?
this line is printed as:
intercept-build: ERROR: Something unexpected had happened.
(and the stack-trace)
because the logging formating. so, 'intercept-build' and 'error' will be part of the message anyway.
================
Comment at: tools/scan-build-py/libscanbuild/intercept.py:98
@@ +97,3 @@
+
+ if args.override_compiler or not ear_library_path:
+ environment.update({
----------------
dcoughlin wrote:
> What is the purpose of setting CC/CXX environmental variables here? Doesn't libear intercept calls to the compiler? Why is intercept-cc/cxx needed?
documentation added. this branch is used when preload does not work. (eg.: windows builds or failed to compile `libear` for unknown reasons.)
================
Comment at: tools/scan-build-py/libscanbuild/runner.py:63
@@ +62,3 @@
+ 'error_type', 'error_output', 'exit_code'])
+def report_failure(opts):
+ """ Create report when analyzer failed.
----------------
dcoughlin wrote:
> If it is hard to make sure that opts has the right dictionary keys, maybe it might be better to create a value class with methods?
`require` decorator added to ensure the keys. (create class would be bloat in my view.)
================
Comment at: tools/scan-build-py/libscanbuild/runner.py:113
@@ +112,3 @@
+ at require(['clang', 'analyze', 'directory', 'output'])
+def run_analyzer(opts, continuation=report_failure):
+ """ It assembles the analysis command line and executes it. Capture the
----------------
dcoughlin wrote:
> What is the point of "continuation"?
this module use continuation to pass the current state and the method to call. i find this abstraction much better than create class methods (or a single method as it in the Perl implementation). Specially better if you want to test these methods!
================
Comment at: tools/scan-build-py/libscanbuild/shell.py:1
@@ +1,2 @@
+# -*- coding: utf-8 -*-
+# The LLVM Compiler Infrastructure
----------------
dcoughlin wrote:
> I'm surprised there is not a library to do this.
>
> Also, maybe a better name for this module is "shell_escaping".
the system library package called `shlex` and it has only the decode method. and failed on many of the real life tests. (in previous version i used that.)
i don't find `shell_escaping` a _better_ name. specially when i think how the method names would be: `shell_escaping.encode`. i think `shell.encode` is more descriptive.
================
Comment at: tools/scan-build-py/setup.py:1
@@ +1,2 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
----------------
dcoughlin wrote:
> Is this needed in the clang repo?
in order to make this code a standalone python tool tool, we need this file. (see llvm/utils/lit directory for example.)
http://reviews.llvm.org/D9600
More information about the cfe-commits
mailing list