dcoughlin added a comment.
Thanks Laszlo!
Is there a more descriptive name than "intercept-build" (I realize scan-build
is pretty general too). It seems to me the point of the intercept-build tool is
to generate the compilation database. I think it would be helpful if the tool
name indicated that rather than the *method* used to to generate the database.
I don't have any great suggestions: "compilation-database-build", "log-build",
"log-compilation-build", ...
A couple more comments inline.
================
Comment at: tools/scan-build-py/MANIFEST.in:1
@@ +1,2 @@
+include README.md
+include *.txt
----------------
rizsotto.mailinglist wrote:
> 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.)
Can you explain why this is needed? We didn't need one for the perl scan-build
nor for SATestBuild.py/SATestAdd.py?
A difference between lit and scan-build is that scan-build is not a standalone
tool. Are you envisioning users installing scan-build without installing clang?
================
Comment at: tools/scan-build-py/libear/__init__.py:1
@@ +1,2 @@
+# -*- coding: utf-8 -*-
+# The LLVM Compiler Infrastructure
----------------
jroelofs wrote:
> rizsotto.mailinglist wrote:
> > 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.
> I think the best way forward would be to teach the CMake build how to run
> `setup.py`. Then this would work both with and without installation, and
> it'd use the same code paths for both.
> previously you were asking make it work without installation.
Yes. It is very important to support that workflow since many users have
multiple versions of clang on their systems at the same time.
> this file makes it possible to compile the ear library compiled before the
> build runs to use as preloaded library.
Shouldn't this be done as part of the build process and not as part of
installation?
Can the interception library (eventually) be built as part of the global CMake
build? It seems quite fragile to have custom build logic in a python file.
================
Comment at: tools/scan-build-py/libear/__init__.py:1
@@ +1,2 @@
+# -*- coding: utf-8 -*-
+# The LLVM Compiler Infrastructure
----------------
dcoughlin wrote:
> jroelofs wrote:
> > rizsotto.mailinglist wrote:
> > > 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.
> > I think the best way forward would be to teach the CMake build how to run
> > `setup.py`. Then this would work both with and without installation, and
> > it'd use the same code paths for both.
> > previously you were asking make it work without installation.
>
> Yes. It is very important to support that workflow since many users have
> multiple versions of clang on their systems at the same time.
>
> > this file makes it possible to compile the ear library compiled before the
> > build runs to use as preloaded library.
>
> Shouldn't this be done as part of the build process and not as part of
> installation?
>
> Can the interception library (eventually) be built as part of the global
> CMake build? It seems quite fragile to have custom build logic in a python
> file.
jroelofs wrote:
> I think the best way forward would be to teach the CMake build how to run
> setup.py
What is the benefit of running `setup.py` and treating scan-build as a python
package? It seems to me that the fact that scan-build is written in python
should be an implementation detail. Most importantly, installing scan-build
shouldn't muck up the user's python environment.
In my opinion, it would be more user-friendly to install into the new
scan-build install hierarchy that jroelofs recently added. This way all the
scan-build stuff is under one directory that the user can blow away rather than
hidden deep in the bowels of some python environment. This makes it much easier
to ensure that scan-build and clang are in sync and would prevent the user from
accidentally installing scan-build into a virtualenv (which doesn't make sense
because clang doesn't live there).
================
Comment at: tools/scan-build-py/libscanbuild/driver.py:1
@@ +1,2 @@
+# -*- coding: utf-8 -*-
+# The LLVM Compiler Infrastructure
----------------
rizsotto.mailinglist wrote:
> 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.
How about naming the files containing entry points after the name of the
command-line tool that that invokes the entry point? Or moving the entry-point
code to the command-line tool so only shared code is in modules? As it stands,
it is hard to tell what code is called by what tool.
================
Comment at: tools/scan-build-py/libscanbuild/intercept.py:98
@@ +97,3 @@
+
+ if args.override_compiler or not ear_library_path:
+ environment.update({
----------------
rizsotto.mailinglist wrote:
> 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.)
Is there an indication to the user that preload didn't work? Do you think there
should be one? (Or is this an implementation detail that users do not need to
be aware of?)
================
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
----------------
rizsotto.mailinglist wrote:
> 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!
It would be better to decompose larger tasks into individually testable
subtasks and test the output of each of these individually. There is no need
for continuations here. Please remove them.
================
Comment at: tools/scan-build-py/libscanbuild/shell.py:1
@@ +1,2 @@
+# -*- coding: utf-8 -*-
+# The LLVM Compiler Infrastructure
----------------
rizsotto.mailinglist wrote:
> 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.
I found it confusing that the shell module didn't call the shell. You module
comment says: "This module implements basic shell escaping/unescaping methods."
Please use a name for the file that communicates that intent. It doesn't haven
to be "shell_escaping", but "shell" is not descriptive enough. This is
important so that someone who is unfamiliar with the code base can quickly tell
whether the file is relevant to the task at hand.
http://reviews.llvm.org/D9600
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
