[PATCH] D9600: Add scan-build python implementation
Anna Zaks via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 9 13:58:38 PST 2015
zaks.anna added a comment.
Hi Laszlo,
Here are some comments from me.
Should we be worried about the name conflicts (between old scan-build and this tool) during rollout? I think it would be beneficial to rename the tools, but let's discuss the names later. (If we integrate Codecheck, that will affect naming too.)
The libear library should be built as part of clang cmake build; looks like @jroelofs offered to do this.
We should also integrate your tests into lit, which is the way we test all of the other tools in clang.
================
Comment at: tools/scan-build-py/README.md:1
@@ +1,2 @@
+[](https://travis-ci.org/rizsotto/scan-build)
+
----------------
What's this? Please, remove.
================
Comment at: tools/scan-build-py/README.md:6
@@ +5,3 @@
+
+It's a static analyzer wrapper for [Clang][1]. The original `scan-build`
+is written in Perl. This package contains reimplementation of that scripts
----------------
I do not think you need to mention the old scan-build. I'd just describe the tool. Here is a slightly modified copy and paste from scan-build:
"A package designed to wrap a build so that all calls to gcc/clang are intercepted and logged into a compilation database [2] and/or piped to the clang static analyzer. Includes intercept-build tool, which logs the build, as well as scan-build tool, which logs the build and runs the clang static analyzer on it."
I have a bunch of comments about other parts of the documentation. However, I can rewrite parts of it as a separate commit. (It will be more efficient than going back and forth.)
================
Comment at: tools/scan-build-py/README.md:59
@@ +58,3 @@
+
+1. Use compiler wrappers to make actions.
+ The compiler wrappers does run the real compiler and the analyzer.
----------------
This is a good place to point out how each mode can be activated. (Which parameters should be used.)
================
Comment at: tools/scan-build-py/README.md:85
@@ +84,3 @@
+is available from the dynamic loader. On OSX System Integrity Protection security
+feature enabled prevents library preload, so this method will not work in such
+environment.
----------------
Would be good to find out which specific binaries used by the build we are not allowed to interpose on. It would be very unfortunate if this does not work with the System Integrity Protection feature, which is turned on by default.
================
Comment at: tools/scan-build-py/README.md:114
@@ +113,3 @@
+
+The project is licensed under University of Illinois/NCSA Open Source License.
+
----------------
Please, refer to the LICENSE.TXT file like you do in the other files.
================
Comment at: tools/scan-build-py/bin/analyze-c++:1
@@ +1,2 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
----------------
What calls this script?
================
Comment at: tools/scan-build-py/bin/analyze-cc:14
@@ +13,2 @@
+from libscanbuild.analyze import wrapper
+sys.exit(wrapper(False))
----------------
It is hard to figure out/search which functions actually get called from the top level tools. Could you rename all of the public functions so that they have unique names? For example, "wrapper" -> "scan_build_wrapper". This would greatly improve searchability!
================
Comment at: tools/scan-build-py/libear/ear.c:406
@@ +405,3 @@
+ if (0 == fd) {
+ perror("bear: fopen");
+ exit(EXIT_FAILURE);
----------------
It will be hard for the users to interpret these error messages; especially if the tool is widely distributed. (Can be addressed after the initial commit.)
================
Comment at: tools/scan-build-py/libscanbuild/__init__.py:11
@@ +10,3 @@
+This work is derived from the original 'scan-build' Perl implementation and
+from an independent project 'bear'. """
+
----------------
Not sure if this comment helps to understand the functionality, please, remove.
================
Comment at: tools/scan-build-py/libscanbuild/analyze.py:55
@@ +54,3 @@
+ exit_code = capture(args, bin_dir)
+ # next step to run the analyzer against the captured commands
+ if need_analyzer(args.build):
----------------
http://llvm.org/docs/CodingStandards.html#commenting
================
Comment at: tools/scan-build-py/libscanbuild/intercept.py:71
@@ +70,3 @@
+
+ def post_processing(commands):
+ # run post processing only if that was requested
----------------
What does post_processing do? Looks like it allows to append to the compilation database and more.. Could you add a comment or rename the function?
================
Comment at: tools/scan-build-py/libscanbuild/intercept.py:287
@@ +286,3 @@
+ action='store_true',
+ help="""Disable filter, unformated output.""")
+
----------------
Do you explain what the "filter" means somewhere visible to the user? When is filter useful and when it is not?
================
Comment at: tools/scan-build-py/libscanbuild/report.py:6
@@ +5,3 @@
+# License. See LICENSE.TXT for details.
+""" This module is responsible to generate the "cover" report.
+
----------------
"to generate the "cover" report" -> "to generate index.html for the report"
================
Comment at: tools/scan-build-py/libscanbuild/report.py:9
@@ +8,3 @@
+The input for this step is the output directory, where individual reports
+could be found. It parses those reports and generates a final HTML "cover"
+report. """
----------------
"a final HTML "cover" report" -> "index.html"
http://reviews.llvm.org/D9600
More information about the cfe-commits
mailing list