On 11/21/15 9:50 AM, Devin Coughlin wrote:
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.
Oh, good point. This is also important if we're cross building the
toolchain... this library should get built with the same compiler that's
building clang.
================ 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
--
Jon Roelofs
jonat...@codesourcery.com
CodeSourcery / Mentor Embedded
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
