[Impala-ASF-CR] [experimental] Clang Tidy Diff trial balloon

["Jim Apple (Code Review)" <ge...@cloudera.org>]

Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/9751 )

Change subject: [experimental] Clang Tidy Diff trial balloon
......................................................................


Patch Set 2:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/9751/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9751/2//COMMIT_MSG@7
PS2, Line 7: experimental
Can you file a ticket for this, and post a link to this on there? That way, even if this patch doesn't land, they'll be a searchable record of this patch for the next person to play with.


http://gerrit.cloudera.org:8080/#/c/9751/2//COMMIT_MSG@29
PS2, Line 29: the clang-tidy diff
I don't think I understand this phrase. What thing includes the comments? The output of the tool, or some source code file in this patch?


http://gerrit.cloudera.org:8080/#/c/9751/2/bin/clang_tidy_diff.py
File bin/clang_tidy_diff.py:

http://gerrit.cloudera.org:8080/#/c/9751/2/bin/clang_tidy_diff.py@57
PS2, Line 57: def run_tidy(sha="HEAD", is_rev_range=False):
Could you add docstrings for functions in this file?


http://gerrit.cloudera.org:8080/#/c/9751/2/bin/clang_tidy_diff.py@91
PS2, Line 91:         m = re.match(r"^(.+?):(\d+):\d+: ((?:warning|error): .+)$", w, re.MULTILINE | re.DOTALL)
nit: long line, here and elsewhere. You might find https://pypi.python.org/pypi/pep8 useful. I use https://pypi.python.org/pypi/autopep8 with emacs and

    (defun pep8-region (&optional b e)
      (interactive "r")
      (call-process-region b e
                       "/home/jbapple/.local/bin/autopep8" t t nil
                       "--indent-size=2" "--max-line-length=90" "-a" "-a"
                       "-a" "-a" "-a" "-a" "-a" "-a" "--experimental" "-"))


http://gerrit.cloudera.org:8080/#/c/9751/2/bin/clang_tidy_diff.py@139
PS2, Line 139: class TestClangTidyGerrit(unittest.TestCase):
Does anything exercise this?


http://gerrit.cloudera.org:8080/#/c/9751/2/bin/clang_tidy_diff.py@178
PS2, Line 178:                         help="Whether the revision specifies the 'rev..' range")
What does that mean?


http://gerrit.cloudera.org:8080/#/c/9751/2/bin/tidy-diff.sh
File bin/tidy-diff.sh:

http://gerrit.cloudera.org:8080/#/c/9751/2/bin/tidy-diff.sh@28
PS2, Line 28: LAST_COMMITTED_REV=$($ROOT/bin/get-upstream-commit.sh)
Can you make it so this can be explicitly set in the CLI call to tidy-diff.sh?



-- 
To view, visit http://gerrit.cloudera.org:8080/9751
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2fb6a13400367fd3d12a4738bbb2dfc944466a7
Gerrit-Change-Number: 9751
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sun, 01 Apr 2018 03:10:06 +0000
Gerrit-HasComments: Yes