Nomenclature fixes and some TODO cleanup #856

whisperity · 2017-08-31T14:24:58Z

Implements parts of #745 and related issues.

For now, I've added many comments so we can individually see the changes. Of course this can be squashed later.

Xazax-hun

Nice work! Most of my comments related to possible additional cleanups. Only a few questions for these changes.

Xazax-hun · 2017-09-04T13:23:09Z

api/report_server.thrift

@@ -21,58 +21,55 @@ const string API_VERSION = '6.0'
 const i64 MAX_QUERY_SIZE = 500
 //=================================================

-//-----------------------------------------------------------------------------
-struct RunData{
+struct RunData {
  1: i64            runId,        // unique id of the run


Maybe we could adopt the convention of having full sentences in comments to comply with the rest of the codebase as the part of this revamp?

Xazax-hun · 2017-09-04T13:23:48Z

api/report_server.thrift

-  9: shared.Severity     severity         // checker severity
-  10: ReviewData          review           // bug review status informations.
+struct ReportData {
+  1: string                  checkerId,       // the qualified id of the checker that reported this


Maybe this should be renamed as this is the name of the check.

Xazax-hun · 2017-09-04T13:24:21Z

api/report_server.thrift

+  7: i64                     line,            // line number or the reports main section (not part of the path)
+  8: i64                     column,          // column number of the report main section (not part of the path)
+  9: shared.Severity         severity         // checker severity
+  10: ReviewData             review           // bug review status informations.


I would prefer to call this reviewStatus.

Xazax-hun · 2017-09-04T13:27:02Z

api/report_server.thrift

+struct ReportData {
+  1: string                  checkerId,       // the qualified id of the checker that reported this
+  2: string                  bugHash,         // This is unique id of the concrete report.
+  3: string                  checkedFile,     // this is a filepath


In case this is the main file which was given for the analyzer, we should use the same nomenclature.

Xazax-hun · 2017-09-04T13:30:00Z

api/report_server.thrift

@@ -93,7 +90,7 @@ typedef list<ReportFilter> ReportFilterList
 * Members of this struct are interpreted in "OR" relation with each other.


I think this comment is very confusing. Both sentences referring to the members of the struct. Is this correct?
Why do we need the v1? Can we remove that now?

As a side note, I am not sure this has the same expressivity as the old version.

E.g. how do you represent:

(filepath == A AND checkerName == B) OR (severity == C and reviewStatus == D)

Note that the old format was basically a disjunctive normal form, so we could be sure that every predicate could be translated to that.

v1 can only be removed if the command-line client is upgraded to use the same, new, "v2" filtering semantics.

(filepath == A AND checkerName == B) OR (severity == C and reviewStatus == D)
This kind of filter was not explicable by any of the clients in the v1 version either, even if the API -- if directly called -- seemed to had supported that.

@gyorb, @csordasmarton rewrote the filtering system.

The old ReportFilter will be removed.

Xazax-hun · 2017-09-04T13:40:11Z

libcodechecker/libhandlers/analyze.py

                               required=False,
+                               default=argparse.SUPPRESS,


Why do we need this change?

To not have (default: False) printed for the help message in every argument if the argument is already a boolean, and its help already makes it apparent that "it does something if given, otherwise it does not do something ".

Xazax-hun · 2017-09-04T13:44:15Z

libcodechecker/libhandlers/check.py

        )
        # Some arguments don't have default values.
        # We can't set these keys to None because it would result in an error
        # after the call.
        args_to_update = ['skipfile',
                          'analyzers',
+                          'add_compiler_defaults',


Why can't this have a default value?

As above. Boolean options' help messages usually look better if default: False is not given. The code behaves in a way that if it is not passed, it is considered False.

Xazax-hun · 2017-09-04T13:46:34Z

libcodechecker/libhandlers/quickcheck.py

@@ -322,14 +318,14 @@ def __update_if_key_exists(source, target, key):
            output_path=workspace,
            output_format='plist',
            clean=True,
-            jobs=args.jobs,
-            add_compiler_defaults=args.add_compiler_defaults
+            jobs=args.jobs
        )
        # Some arguments don't have default values.
        # We can't set these keys to None because it would result in an error
        # after the call.
        args_to_update = ['skipfile',


Looks like this list is replicated. In the future, it might be better to refactor it in a way that this list is created together with the arg parsers.

gyorb · 2017-09-04T16:36:58Z

api/report_server.thrift

  1: i64            runId,        // unique id of the run
  2: string         runDate,      // date of the run
  3: string         name,         // human readable name of the run
  4: i64            duration,     // duration of the run; -1 if not finished
  5: i64            resultCount,  // number of results in the run
  6: string         runCmd,       // the used check command
-  7: optional bool  can_delete    // true if codeCheckerDBAccess::removeRunResults()
+  7: optional bool  canDelete,    // true if codeCheckerDBAccess::removeRunResults()


Do we still need the canDelete? I'm not sure we use it in the database with the new schema.

gyorb · 2017-09-04T16:38:41Z

api/report_server.thrift

@@ -93,7 +90,7 @@ typedef list<ReportFilter> ReportFilterList
 * Members of this struct are interpreted in "OR" relation with each other.


The old ReportFilter will be removed.

whisperity · 2017-09-06T10:14:29Z

If everything is fine, I'll squash these commits! Don't merge beforehand.

- The 'plist' command which was undocumented for a few releases now has been removed permanently. - Argument list and help of many commands have been made better - The "old-style" 'checkers' command is dropped in favour of the new one introduced back in 5.8. - Most 'cmd' subcommands take run-name as a positional argument, rather than "--name RUN_NAME". - Thrift files have been reformatted to a new scheme of comments and naming.

Xazax-hun

LGTM!

whisperity added enhancement 🌟 WARN ⚠️: Backward compatibility breaker! MIND THE GAP! Merging this patch will mess up compatibility with the previous releases! labels Aug 31, 2017

whisperity added this to the 6.0 pre4 milestone Aug 31, 2017

whisperity added the WIP 💣 Work In Progress label Aug 31, 2017

whisperity changed the title ~~Nomenclature fixes~~ Nomenclature fixes and TODO cleanup Aug 31, 2017

whisperity requested a review from gyorb August 31, 2017 16:28

whisperity changed the title ~~Nomenclature fixes and TODO cleanup~~ Nomenclature fixes and some TODO cleanup Aug 31, 2017

whisperity added API change 📄 Content of patch changes API! CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands database 🗄️ Issues related to the database schema. labels Aug 31, 2017

whisperity force-pushed the nomenclature branch 2 times, most recently from dc446db to 9fc6237 Compare August 31, 2017 19:28

whisperity mentioned this pull request Sep 1, 2017

Drop support for guessing check name #607

Closed

Xazax-hun reviewed Sep 4, 2017

View reviewed changes

gyorb reviewed Sep 4, 2017

View reviewed changes

whisperity force-pushed the version6 branch from 14a3ed2 to 4c8f84d Compare September 5, 2017 08:09

whisperity changed the base branch from version6 to master September 5, 2017 08:59

whisperity force-pushed the nomenclature branch from 9fc6237 to 71939ac Compare September 5, 2017 09:06

gyorb approved these changes Sep 6, 2017

View reviewed changes

whisperity force-pushed the nomenclature branch from 4d0de47 to f2003cc Compare September 6, 2017 10:40

Xazax-hun approved these changes Sep 6, 2017

View reviewed changes

Xazax-hun merged commit 94dec68 into Ericsson:master Sep 6, 2017

whisperity deleted the nomenclature branch September 14, 2017 07:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nomenclature fixes and some TODO cleanup #856

Nomenclature fixes and some TODO cleanup #856

whisperity commented Aug 31, 2017 •

edited

Loading

Xazax-hun left a comment

Xazax-hun Sep 4, 2017

Xazax-hun Sep 4, 2017

Xazax-hun Sep 4, 2017

Xazax-hun Sep 4, 2017

Xazax-hun Sep 4, 2017

Xazax-hun Sep 4, 2017

whisperity Sep 4, 2017

gyorb Sep 4, 2017

Xazax-hun Sep 4, 2017

whisperity Sep 4, 2017

Xazax-hun Sep 4, 2017

whisperity Sep 4, 2017

Xazax-hun Sep 4, 2017

gyorb Sep 4, 2017

gyorb Sep 4, 2017

whisperity commented Sep 6, 2017

Xazax-hun left a comment

		@@ -93,7 +90,7 @@ typedef list<ReportFilter> ReportFilterList
		* Members of this struct are interpreted in "OR" relation with each other.

Nomenclature fixes and some TODO cleanup #856

Nomenclature fixes and some TODO cleanup #856

Conversation

whisperity commented Aug 31, 2017 • edited Loading

Xazax-hun 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

whisperity commented Sep 6, 2017

Xazax-hun left a comment

Choose a reason for hiding this comment

whisperity commented Aug 31, 2017 •

edited

Loading