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 @@
+@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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
