rizsotto.mailinglist added a comment.
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.
================
Comment at: tools/codechecker/bin/CodeChecker:35
@@ +34,3 @@
+
+ python = os.path.join('python')
+ common_lib = os.path.join(package_root,
----------------
wow, that looks woodoo. can you explain (in code comment) why is that necessary?
================
Comment at: tools/codechecker/bin/CodeChecker:67
@@ +66,3 @@
+
+ original_env = os.environ.copy()
+ checker_env = original_env
----------------
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?
================
Comment at: tools/codechecker/bin/CodeChecker:70
@@ +69,3 @@
+
+ tmp_dir = tempfile.mkdtemp()
+
----------------
can you backport `tempfile.TemporaryDirectory` (from python 3.2) in one of your
modules and use it inside `with` for proper resource guarding?
================
Comment at: tools/codechecker/bin/CodeChecker:85
@@ +84,3 @@
+ pid = os.getpid()
+ for p in procPool:
+ os.kill(p, signal.SIGINT)
----------------
why not store the `subprocess.Popen` object and send signal via `send_signal`?
(that would make it more portable too.)
================
Comment at: tools/codechecker/libcodechecker/build_manager.py:30
@@ +29,3 @@
+ """
+ proc = subprocess.Popen(command,
+ bufsize=-1,
----------------
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?
================
Comment at: tools/codechecker/libcodechecker/build_manager.py:55
@@ +54,3 @@
+
+ try:
+ original_env_file = os.environ['CODECHECKER_ORIGINAL_BUILD_ENV']
----------------
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?
================
Comment at: tools/codechecker/libcodechecker/build_manager.py:131
@@ +130,3 @@
+ return None
+ except AttributeError as ex:
+ # args.log_file was not set
----------------
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.
================
Comment at: tools/codechecker/libcodechecker/build_manager.py:150
@@ +149,3 @@
+
+ if intercept_build_executable == None:
+ if platform.system() == 'Linux':
----------------
`if intercept_build_executable is None:` would be more python.
================
Comment at: tools/codechecker/libcodechecker/build_manager.py:169
@@ +168,3 @@
+
+ open(log_file, 'a').close() # same as linux's touch
+
----------------
the comment is not really helpful!
why does it need a touch? or you just create an empty one? what for?
================
Comment at: tools/codechecker/libcodechecker/build_manager.py:177
@@ +176,3 @@
+ except AttributeError as aerr:
+ LOG.error(aerr)
+ sys.exit(1)
----------------
`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?
https://reviews.llvm.org/D24040
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
