Local compare mode added to CodeChecker cmd diff mode. #723

dkrupp · 2017-07-12T15:20:39Z

fixes #692

whisperity

Code looks good, seems to work well, thank you.

Only minor formatting issues. Also, please update docs/user_guide.md with the new help of cmd diff (that includes --reportdir), and maybe with an example of this use case.

whisperity · 2017-07-13T06:36:03Z

libcodechecker/cmd/cmd_line_client.py

@@ -8,6 +8,7 @@
 import getpass
 import json
 import sys
+import os


Import order. os > sys.

whisperity · 2017-07-13T06:36:27Z

libcodechecker/cmd/cmd_line_client.py

@@ -19,6 +20,8 @@
 from libcodechecker import suppress_file_handler
 from libcodechecker.logger import LoggerFactory
 from libcodechecker.output_formatters import twodim_to_str
+from libcodechecker.analyze import plist_parser


Import order here too. Alphabetical on module name: libcodechecker, libcodechecker.analyze, libcodechecker.logger, etc.

whisperity · 2017-07-13T06:41:47Z

libcodechecker/cmd/cmd_line_client.py

-
-        sort_type = None
+        sort_mode = []
+        sort_mode.append(codeCheckerDBAccess.ttypes.SortMode(


Okay, I agree that this long line is very ugly here. Also, expression can be simplified. Let's do this:

Introduce a new import at the top:

import codeCheckerDBAccess import shared from Authentication import ttypes as AuthTypes + from codeCheckerDBAccess import ttypes

Then rewrite this statement so the qualified name is only ttypes.SortMode and such.

Empty list and append can be simplified into a single list constructor: sort_mode = [ttypes.SortMode(...)].

whisperity · 2017-07-13T06:42:56Z

libcodechecker/cmd/cmd_line_client.py

+        add_filter_conditions(report_filter[0], args.filter)
+
+        sort_mode = []
+        sort_mode.append(codeCheckerDBAccess.ttypes.SortMode(


(Same simplification as in the other function.)

whisperity · 2017-07-13T06:46:52Z

libcodechecker/cmd/cmd_line_client.py

+        return ""
+
+    def getLineFromRemoteFile(client, fid, lineno):
+        # FIXME: large special file content


Maybe we should mention that "special" file content in this case is the UTF-8 encoding problem?

whisperity · 2017-07-13T06:47:07Z

libcodechecker/cmd/cmd_line_client.py

+
+    def getLineFromRemoteFile(client, fid, lineno):
+        # FIXME: large special file content
+        # cause connection abort


Also this comment is not formatted as a full sentence.

whisperity · 2017-07-13T06:48:15Z

libcodechecker/cmd/cmd_line_client.py

+        return all_reports
+
+    def getLineFromFile(filename, lineno):
+        with open(filename, 'r') as f:


We should introduce a comment here that due to potentially big files, this verbose algorithm is much more efficient than return f.readlines()[lineno]. (I have checked some posts on the Internet about this, and yes, it's more efficient this way, even if it looks more down-to-Earth.)

i wouldnt spoil the code with such comments.

whisperity · 2017-07-13T06:49:51Z

libcodechecker/cmd/cmd_line_client.py

+        for rep in new_results:
+            bughash = rep.main['issue_hash_content_of_line_in_context']
+            new_hashes[bughash] = rep
+        # show new reports from the report dir


Please move the comments into the branch's body, and format them as full sentence.

whisperity · 2017-07-13T06:50:50Z

libcodechecker/cmd/cmd_line_client.py

+                if not (result.bugHash in new_hashes):
+                    filtered_reports.append(result)
+        # show bugs in the report dir
+        # which is not suppressed in the baseline (server)


"unresolved" no longer "overrides" behaviour on whether or not suppressed results are grabbed, as we reviewed it earlier.

whisperity · 2017-07-13T06:52:09Z

libcodechecker/cmd/cmd_line_client.py

-                    args.output_format)
+    results = []
+    if 'reportdir' in args:
+        if 'newname' in args:


argparse can handle mutually exclusive argument groups.

bruntib · 2017-07-13T10:10:32Z

libcodechecker/cmd/cmd_line_client.py

+                if i == lineno:
+                    return line
+                i += 1
+        return ""


You don't need a loop here:
return "" if len(lines) < lineno else lines[lineno - 1]

bruntib · 2017-07-13T10:36:02Z

libcodechecker/cmd/cmd_line_client.py

+                i += 1
+        return ""
+
+    def getDiffReportDir(getterFn, baseid, report_dir, suppr, diff_type):


The first parameter of this function doesn't seem necessary. This function is called once in the project where it gets client.getRunResults, but anyway this will always be used for getting the reports. The client object definition in line 425 could be replaced above this function (e.g. as the first statement of handle_diff_results() in line 266) which is used here.

I followed the calling style of other functions in this file, so I think i will leave it like this.

…ot the same ones).

whisperity · 2017-07-13T13:46:43Z

tests/libtest/codechecker.py

+    except CalledProcessError as cerr:
+        print("Failed to call:\n" + ' '.join(cerr.cmd))
+        return cerr.returncode
+


There needs to be 2 linebreaks here instead of 1, this breaks pep8.

whisperity · 2017-07-13T13:46:55Z

libcodechecker/cmd/cmd_line_client.py

 import sys

+


Extra line-break, please remove.

whisperity · 2017-07-13T13:47:27Z

libcodechecker/cmd/cmd_line_client.py

-
-        sort_type = None
+        sort_mode = []
+        sort_mode.append(ttypes.SortMode(


Statement can be simplified:

sort_mode = [ttypes.SortMode(...)]

whisperity · 2017-07-13T13:48:09Z

libcodechecker/cmd/cmd_line_client.py

+        add_filter_conditions(report_filter[0], args.filter)
+
+        sort_mode = []
+        sort_mode.append(ttypes.SortMode(


sort_mode = [ttypes.SortMode(...)]

whisperity · 2017-07-13T13:52:22Z

tests/functional/diff/__init__.py

@@ -99,13 +101,30 @@ def setup_package():

    test_project_name_new = project_info['name'] + '_' + uuid.uuid4().hex

+    # Let's run the second analysis with different
+    # checkers to have some real difference.
+    codechecker_cfg['checkers'] = ['-d', 'core.StackAddressEscape',


This here looks tricky on first glance, as -d,-e order is kept instead of the checker order. We should change this to:

['-e', 'core.CallAndMessage', '-d', 'core.StackAddressEscape'] so the checker order is kept with the previous code in line 73.

dkrupp force-pushed the compare_mode branch from 136ad0a to d220640 Compare July 12, 2017 15:23

dkrupp requested review from whisperity and bruntib July 12, 2017 15:25

whisperity requested changes Jul 13, 2017

View reviewed changes

dkrupp mentioned this pull request Jul 13, 2017

Multiple products on the same server (multi-database system) [142h] #691

Closed

29 tasks

bruntib reviewed Jul 13, 2017

View reviewed changes

dkrupp added 3 commits July 13, 2017 15:17

Local compare mode added to CodeChecker cmd diff mode.

19c40bf

Changing diff test cases to actually compare to different runs (and n…

2957f2f

…ot the same ones).

Test cases added for local compare mode

ce7344d

dkrupp force-pushed the compare_mode branch from 6413af2 to ce7344d Compare July 13, 2017 13:31

whisperity requested changes Jul 13, 2017

View reviewed changes

Fixing minor comments

ec1d894

dkrupp force-pushed the compare_mode branch from 65cb1e3 to ec1d894 Compare July 13, 2017 15:59

whisperity approved these changes Jul 13, 2017

View reviewed changes

whisperity merged commit 79dff78 into Ericsson:master Jul 13, 2017

This was referenced Jul 13, 2017

Handle a Thrift exception if file content contains Unicode char that escapes as \u01?? by b64ing source text #726

Merged

Update documentation #730

Merged

csordasmarton mentioned this pull request Oct 26, 2017

Local Compare mode HTML output files #1044

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Local compare mode added to CodeChecker cmd diff mode. #723

Local compare mode added to CodeChecker cmd diff mode. #723

dkrupp commented Jul 12, 2017

whisperity left a comment

whisperity Jul 13, 2017

dkrupp Jul 13, 2017

whisperity Jul 13, 2017

dkrupp Jul 13, 2017

whisperity Jul 13, 2017

dkrupp Jul 13, 2017

whisperity Jul 13, 2017

dkrupp Jul 13, 2017

whisperity Jul 13, 2017

dkrupp Jul 13, 2017

whisperity Jul 13, 2017

dkrupp Jul 13, 2017

whisperity Jul 13, 2017

dkrupp Jul 13, 2017

whisperity Jul 13, 2017

dkrupp Jul 13, 2017

whisperity Jul 13, 2017

dkrupp Jul 13, 2017

whisperity Jul 13, 2017

bruntib Jul 13, 2017

dkrupp Jul 13, 2017

bruntib Jul 13, 2017

dkrupp Jul 13, 2017

whisperity Jul 13, 2017

dkrupp Jul 13, 2017

whisperity Jul 13, 2017

dkrupp Jul 13, 2017

whisperity Jul 13, 2017

dkrupp Jul 13, 2017

whisperity Jul 13, 2017

dkrupp Jul 13, 2017

whisperity Jul 13, 2017

dkrupp Jul 13, 2017

Local compare mode added to CodeChecker cmd diff mode. #723

Local compare mode added to CodeChecker cmd diff mode. #723

Conversation

dkrupp commented Jul 12, 2017

whisperity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment