Page MenuHomePhabricator

codechecker tool core
Needs ReviewPublic

Authored by o.gyorgy on Aug 30 2016, 10:01 AM.

Details

Summary

This is the first patch for the codechecker tool. (https://github.com/Ericsson/codechecker/issues/359)

This patch contains only the core files, documentation and unit tests.

  • libcodechecker
  • report_server (handle report storage)
  • report_viewer (handle viewer client requests (web, command line))
  • thrift api (for storing and getting results by the analyzers)
  • unit tests
  • config files (runtime and packaging)
  • command line client to view results

The next patch with functional tests have additional infrastructure requirements (PostgreSQL) which should be further discussed.
The third patch will contain the web GUI part of codechecker.

Diff Detail

Event Timeline

o.gyorgy updated this revision to Diff 69714.Aug 30 2016, 10:01 AM
o.gyorgy retitled this revision from to codechecker tool core.
o.gyorgy updated this object.
NoQ added a subscriber: NoQ.Aug 30 2016, 10:47 AM
r0mai added a subscriber: r0mai.Aug 31 2016, 2:34 AM

Gyorgy and the ericsson team, thanks for doing this. very good job! good targeted functionality. i don't want to underestimate the complexity it requires, but to me this is a giant code. i do miss the explanation of the overall functional description what a module does and how it relates to other modules. i found a gap between the high level overview and the code comments.
generally speaking i found this code under documented and a bit verbose. the comments in the code is not really paint the picture that i need in order to fix a bug, or implement a feature. and it might only be my personal taste, but found that creating types (or classes this case) in a dynamically typed script language, it makes the code very verbose while it does not implement much.
was commented only on two random modules, but found the same patterns in many other ones.
found that this code is not pep8 conform. pylint found a lot of errors and warnings, and a couple of duplicate codes.
i hope we can fix these findings and that would make this code more solid and understood by others.

tools/codechecker/bin/CodeChecker
35

wow, that looks woodoo. can you explain (in code comment) why is that necessary?

67

can you explain (in code comment) why are you playing with the environment? what problem does it solve and why do you need a backup?

70

can you backport tempfile.TemporaryDirectory (from python 3.2) in one of your modules and use it inside with for proper resource guarding?

85

why not store the subprocess.Popen object and send signal via send_signal? (that would make it more portable too.)

tools/codechecker/libcodechecker/build_manager.py
30

is there any reason not to use subprocess.call instead? if you want that to be in silent, you shall pass {'stdout': subprocess.PIPE, 'stderr': subprocess.STDOUT} to it, otherwise it prints the output without you doing the copy. also noticed the shell=True which makes it very non-portable. and the command parameter is a string instead of a list. is there any reason for doing like this?

55

save-to-file and restore-from-file the environment would deserve a separate module i would say. is there any reason why it's not having those utility methods?

131

why not have a command line parameter accessor method, that would either return the value if that was given or None? i found this pattern multiple files. that would get rid of the try block completely.

150

if intercept_build_executable is None: would be more python.

169

the comment is not really helpful!
why does it need a touch? or you just create an empty one? what for?

177

logger.exception is logging the exception value implicitly at ERROR level too. but what i'm trying to say is, there is no error message in this line. you just throw the exception into the user face. can you add some explanation or instruction for the user?

artem.tamazov resigned from this revision.Aug 31 2016, 7:38 AM
artem.tamazov removed a reviewer: artem.tamazov.

It seems that my name got into reviewers list by mistake.

This looks like a fairly large tool. Should it get its own "subproject level" directory in the SVN instead of being nested within clang?

tools/codechecker/README.md
50

Is it outdated?

This looks like a fairly large tool. Should it get its own "subproject level" directory in the SVN instead of being nested within clang?

I'd add that the clang-tools-extra are more closely tied to clang than what this seems to be. Is there a strong rev-lock with clang that I missed?

szepet added a subscriber: szepet.Aug 31 2016, 11:22 PM

Gyorgy and the ericsson team, thanks for doing this. very good job! good targeted functionality. i don't want to underestimate the complexity it requires, but to me this is a giant code. i do miss the explanation of the overall functional description what a module does and how it relates to other modules. i found a gap between the high level overview and the code comments.

Would that help If update architecture.md documentation (which gives a high level overview) to be more in sync with the source code, with additional comments and descriptions in the modules?

generally speaking i found this code under documented and a bit verbose. the comments in the code is not really paint the picture that i need in order to fix a bug, or implement a feature. and it might only be my personal taste, but found that creating types (or classes this case) in a dynamically typed script language, it makes the code very verbose while it does not implement much.

We use classes to handle and setup environment, build, analyzer and other configurations. In the infrastructure this makes available to easily introduce new analyzers and handle the output of them. Multiple output handling is supported for each analyzer (print to stdout, store to database).

was commented only on two random modules, but found the same patterns in many other ones.
found that this code is not pep8 conform. pylint found a lot of errors and warnings, and a couple of duplicate codes.
i hope we can fix these findings and that would make this code more solid and understood by others.

I will address these issues and update our code.

Thanks for the feedback.

This looks like a fairly large tool. Should it get its own "subproject level" directory in the SVN instead of being nested within clang?

I'd add that the clang-tools-extra are more closely tied to clang than what this seems to be. Is there a strong rev-lock with clang that I missed?

I've put it here because it is similar to scan-build/scan-build-py and on OSX it uses the intercept-build from scan-build-py. Right now there is an alternative logger on linux (not in this patch) but we plan to move to use only the intercept-build.
We process the output generated by clang and clang-tidy and we use the command line options for these tools which could change with newer revisions, in that case this tool needs to be updated too.

I will update the readme too.

Gyorgy and the ericsson team, thanks for doing this. very good job! good targeted functionality. i don't want to underestimate the complexity it requires, but to me this is a giant code. i do miss the explanation of the overall functional description what a module does and how it relates to other modules. i found a gap between the high level overview and the code comments.

Would that help If update architecture.md documentation (which gives a high level overview) to be more in sync with the source code, with additional comments and descriptions in the modules?

i prefer python docstrings instead of the markdown files. found that the __init__.py files are empty. those are good candidates for a higher overview of the package architecture. now module headers are one liners most of the places, which is a missed opportunity to document the connections between the modules. method docstrings are non descriptive enough. (echoing back the function names with spaces.) would read about the usage of the method, contract between the caller and the callee, or the implementation difficulties.

generally speaking i found this code under documented and a bit verbose. the comments in the code is not really paint the picture that i need in order to fix a bug, or implement a feature. and it might only be my personal taste, but found that creating types (or classes this case) in a dynamically typed script language, it makes the code very verbose while it does not implement much.

We use classes to handle and setup environment, build, analyzer and other configurations. In the infrastructure this makes available to easily introduce new analyzers and handle the output of them. Multiple output handling is supported for each analyzer (print to stdout, store to database).

yes, i got that you were using OO. and try to re-use code this way... my point is, this abstraction, in this language, ends up to have an abstract class with empty methods, a couple of implementations and the user of the abstract class, which enjoys the polymorphism. to understand a single implementation i have to read (at least) three different modules/files. it's very fragmented to me... my proposal would be, to write util methods. implement the different analyzer commands as a combination of the util methods. this abstraction fits better to a script language. it also make your code more dense, less verbose, easier to maintain, test or reason about... those was my two cents.

This looks like a fairly large tool. Should it get its own "subproject level" directory in the SVN instead of being nested within clang?

I'd add that the clang-tools-extra are more closely tied to clang than what this seems to be. Is there a strong rev-lock with clang that I missed?

I've put it here because it is similar to scan-build/scan-build-py and on OSX it uses the intercept-build from scan-build-py. Right now there is an alternative logger on linux (not in this patch) but we plan to move to use only the intercept-build.
We process the output generated by clang and clang-tidy and we use the command line options for these tools which could change with newer revisions, in that case this tool needs to be updated too.

By strong rev-lock, I meant "Using the internal C++ API" of clang itself.
The scan-build.py thing seems to me like a tiny script compared to CodeChecker.