dcoughlin added a comment. Hi Laszlo, thanks for the update!
Some high-level questions/comments (and various small things I noticed inline). - I tried running scan-build in interposition mode (i.e., uncommenting out #"$SCRIPT_DIR/analyze-build" $@ in scan-build) and got python compile errors. When you did your testing to compare output with the old scan-build, did you use this mode? - When I run scan-build in intercept-build mode on openssl-1.0 on OS X I get compile errors. Is this one of the projects you tested with? When I run in it in analyze-build mode I get diffs with CmpRuns.py -- it looks like might be because some paths are absolute. What kind of fidelity with the old scan-build do expect at this point? - What is the point of intercept-cc/intercept-c++? I thought libear didn't need to set CC/CXX environmental variables. - Do all the files in this patch need to be committed into the clang repo? There seem to be some extraneous files (I've asked inline about the ones that don't seem necessary.) - I found it difficult to understand how all the modules and command-line tools fit together. Can the module names be made more harmonious with the command-line tool names(the scripts in bin). It is not at all obvious which modules belong to which tools, etc. I think it would be good to document this in the code, as well. ================ Comment at: tools/scan-build-py/.travis.yml:1 @@ +1,2 @@ +language: python + ---------------- Is this file needed in clang trunk? ================ Comment at: tools/scan-build-py/CHANGES.txt:1 @@ +1,1 @@ +v<version>, <date> -- Initial release. ---------------- Is this one needed too? ================ Comment at: tools/scan-build-py/MANIFEST.in:1 @@ +1,2 @@ +include README.md +include *.txt ---------------- How about this one? Is it needed in clang trunk? ================ Comment at: tools/scan-build-py/README.md:70 @@ +69,3 @@ +--------------- +If you find a bug in this documentation or elsewhere in the program or would +like to propose an improvement, please use the project's [github issue ---------------- This should probably point to llvm bugzilla. https://llvm.org/bugs/enter_bug.cgi?product=clang ================ Comment at: tools/scan-build-py/bin/scan-build:13 @@ +12,3 @@ +# +#"$SCRIPT_DIR/analyze-build" $@ +#"$SCRIPT_DIR/intercept-build" all $@ ---------------- These should be controlled by flags rather than by commenting the required behavior in or out. I also think analyze-build should be the default behavior, to maintain compatibility with the existing scan-build. ================ Comment at: tools/scan-build-py/libear/__init__.py:1 @@ +1,2 @@ +# -*- coding: utf-8 -*- +# The LLVM Compiler Infrastructure ---------------- 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? ================ Comment at: tools/scan-build-py/libear/__init__.py:79 @@ +78,3 @@ +class Context(object): + """ Abstract class to represent different toolset. """ + ---------------- Maybe "Toolset" would be a more descriptive name than "Context"? ================ Comment at: tools/scan-build-py/libear/__init__.py:99 @@ +98,3 @@ + def dl_libraries(self): + pass + ---------------- 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. ================ Comment at: tools/scan-build-py/libear/__init__.py:151 @@ +150,3 @@ +@contextlib.contextmanager +def make_context(src_dir): + platform = sys.platform ---------------- Why is this a generator? There is no functionality after yield. ================ Comment at: tools/scan-build-py/libear/__init__.py:166 @@ +165,3 @@ + self.ctx = context + self.results = {'APPLE': sys.platform == 'darwin'} + ---------------- What does this do? Why is it hard-coded? ================ Comment at: tools/scan-build-py/libear/__init__.py:224 @@ +223,3 @@ +def do_configure(context): + yield Configure(context) + ---------------- Why is this a generator? ================ Comment at: tools/scan-build-py/libear/__init__.py:262 @@ +261,2 @@ +def create_shared_library(name, context): + yield SharedLibrary(name, context) ---------------- Why is this a generator? ================ Comment at: tools/scan-build-py/libscanbuild/clang.py:33 @@ +32,3 @@ + + This method receives the full command line for direct compilation. And + it generates the command for indirect compilation. """ ---------------- I think it would be more accurate to say that this method returns the front-end invocation that would be executed as a result of the given driver invocation. ================ 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. """ ---------------- 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? ================ Comment at: tools/scan-build-py/libscanbuild/command.py:23 @@ +22,3 @@ + + ignored = { + '-g': 0, ---------------- 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? ================ Comment at: tools/scan-build-py/libscanbuild/command.py:98 @@ +97,3 @@ +def classify_source(filename, cplusplus=False): + """ Return the language from fille name extension. """ + ---------------- Typo "fille". ================ Comment at: tools/scan-build-py/libscanbuild/command.py:123 @@ +122,3 @@ + +def cplusplus_compiler(name): + """ Returns true when the compiler name refer to a C++ compiler. """ ---------------- This method name should probably start with "is_" ================ Comment at: tools/scan-build-py/libscanbuild/driver.py:1 @@ +1,2 @@ +# -*- coding: utf-8 -*- +# The LLVM Compiler Infrastructure ---------------- Why is this file called "driver"? ================ Comment at: tools/scan-build-py/libscanbuild/driver.py:34 @@ +33,3 @@ +def main(bin_dir): + """ Entry point for 'scan-build'. """ + ---------------- Should this be 'intercept-build'? ================ Comment at: tools/scan-build-py/libscanbuild/driver.py:67 @@ +66,3 @@ + except Exception: + logging.exception("Something unexpected had happened.") + return 127 ---------------- I think this error message can be improved. Perhaps "Unexpected error running intercept-build"? ================ Comment at: tools/scan-build-py/libscanbuild/driver.py:105 @@ +104,3 @@ +def need_analyzer(args): + """ Check the internt of the build command. """ + ---------------- Typo "internt". ================ Comment at: tools/scan-build-py/libscanbuild/driver.py:107 @@ +106,3 @@ + + return len(args) and not re.search('configure|autogen', args[0]) + ---------------- This needs documentation. ================ Comment at: tools/scan-build-py/libscanbuild/intercept.py:98 @@ +97,3 @@ + + if args.override_compiler or not ear_library_path: + environment.update({ ---------------- 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? ================ Comment at: tools/scan-build-py/libscanbuild/interposition.py:36 @@ +35,3 @@ + # run the build command + environment = setup_environment(args, target_dir.name, bin_dir) + logging.debug('run build in environment: %s', environment) ---------------- Trying to access target_dir.name here causes a python error. ================ Comment at: tools/scan-build-py/libscanbuild/interposition.py:94 @@ +93,3 @@ + +def wrapper(cplusplus): + """ This method implements basic compiler wrapper functionality. """ ---------------- It would be good to document that this is essentially the "main" of analyze-cc and document what it does. ================ Comment at: tools/scan-build-py/libscanbuild/runner.py:21 @@ +20,3 @@ + +def run(opts): + """ Entry point to run (or not) static analyzer against a single entry ---------------- This needs documentation on what opts can/should contain. ================ 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. ---------------- 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? ================ 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 ---------------- What is the point of "continuation"? ================ Comment at: tools/scan-build-py/libscanbuild/shell.py:1 @@ +1,2 @@ +# -*- coding: utf-8 -*- +# The LLVM Compiler Infrastructure ---------------- I'm surprised there is not a library to do this. Also, maybe a better name for this module is "shell_escaping". ================ Comment at: tools/scan-build-py/setup.py:1 @@ +1,2 @@ +#!/usr/bin/env python +# -*- coding: utf-8 -*- ---------------- Is this needed in the clang repo? http://reviews.llvm.org/D9600 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
