[PATCH] D24040: codechecker tool core

Gyorgy Orban via cfe-commits cfe-commits at lists.llvm.org
Fri Sep 2 03:23:28 PDT 2016


o.gyorgy added a comment.

In https://reviews.llvm.org/D24040#530293, @rizsotto.mailinglist wrote:

> 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.


https://reviews.llvm.org/D24040





More information about the cfe-commits mailing list