You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Kim Jin Chul (Code Review)" <ge...@cloudera.org> on 2017/12/13 13:23:03 UTC
[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
Kim Jin Chul has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8829
Change subject: IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
......................................................................
IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
In the commit 4feb4f3a, the third party library pcg-cpp was excluded
from the clang-tidy check. It could make unexpected side effect, so
fixing some warnings from clang-tidy is better than avoidance of the check.
Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
---
M be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp
M bin/run_clang_tidy.sh
2 files changed, 4 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/8829/1
--
To view, visit http://gerrit.cloudera.org:8080/8829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
Gerrit-Change-Number: 8829
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8829 )
Change subject: IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
......................................................................
IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
In the commit 4feb4f3a, the third party library pcg-cpp was excluded
from the clang-tidy check. It could make unexpected side effect, so
fixing some warnings from clang-tidy is better than avoidance of the
check.
Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
Reviewed-on: http://gerrit.cloudera.org:8080/8829
Reviewed-by: Jim Apple <jb...@apache.org>
Tested-by: Impala Public Jenkins
---
M be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp
M bin/run_clang_tidy.sh
2 files changed, 13 insertions(+), 2 deletions(-)
Approvals:
Jim Apple: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/8829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
Gerrit-Change-Number: 8829
Gerrit-PatchSet: 5
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
Hello Jim Apple,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8829
to look at the new patch set (#2).
Change subject: IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
......................................................................
IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
In the commit 4feb4f3a, the third party library pcg-cpp was excluded
from the clang-tidy check. It could make unexpected side effect, so
fixing some warnings from clang-tidy is better than avoidance of the check.
Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
---
M be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp
M bin/run_clang_tidy.sh
2 files changed, 8 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/8829/2
--
To view, visit http://gerrit.cloudera.org:8080/8829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
Gerrit-Change-Number: 8829
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/8829 )
Change subject: IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
......................................................................
Patch Set 2:
(3 comments)
Thanks for taking care of this! Nice work, as usual.
http://gerrit.cloudera.org:8080/#/c/8829/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8829/2//COMMIT_MSG@11
PS2, Line 11: ck.
nit: long line
http://gerrit.cloudera.org:8080/#/c/8829/2/be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp
File be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp:
http://gerrit.cloudera.org:8080/#/c/8829/2/be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp@37
PS2, Line 37: // To avoid of warning: 'PCG_USE_ZEROCHECK_ROTATE_IDIOM' is not defined, evaluates to 0
Please wrap each in an ifndef block.
http://gerrit.cloudera.org:8080/#/c/8829/2/be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp@599
PS2, Line 599: * warning: expansion of date or time macro is not reproducible
> The struct is not used in our code and pcg-cpp's sample and test codes. I t
nit: can you reword slightly - it's not just that clang-tidy complains, it's that we don't want to introduce non-reproducability into the system, because it makes bugs harder to diagnose.
--
To view, visit http://gerrit.cloudera.org:8080/8829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
Gerrit-Change-Number: 8829
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Comment-Date: Mon, 18 Dec 2017 04:19:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8829 )
Change subject: IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/8829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
Gerrit-Change-Number: 8829
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Comment-Date: Mon, 18 Dec 2017 18:37:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
Hello Jim Apple,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8829
to look at the new patch set (#3).
Change subject: IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
......................................................................
IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
In the commit 4feb4f3a, the third party library pcg-cpp was excluded
from the clang-tidy check. It could make unexpected side effect, so
fixing some warnings from clang-tidy is better than avoidance of the
check.
Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
---
M be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp
M bin/run_clang_tidy.sh
2 files changed, 13 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/8829/3
--
To view, visit http://gerrit.cloudera.org:8080/8829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
Gerrit-Change-Number: 8829
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
Kim Jin Chul has posted comments on this change. ( http://gerrit.cloudera.org:8080/8829 )
Change subject: IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
......................................................................
Patch Set 5:
Jim, thanks for your review!
--
To view, visit http://gerrit.cloudera.org:8080/8829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
Gerrit-Change-Number: 8829
Gerrit-PatchSet: 5
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Comment-Date: Tue, 19 Dec 2017 00:03:12 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
Kim Jin Chul has posted comments on this change. ( http://gerrit.cloudera.org:8080/8829 )
Change subject: IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8829/2/be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp
File be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp:
http://gerrit.cloudera.org:8080/#/c/8829/2/be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp@599
PS2, Line 599: * warning: expansion of date or time macro is not reproducible
The struct is not used in our code and pcg-cpp's sample and test codes. I thought comment out is better than removal of the code because someone needs this later.
--
To view, visit http://gerrit.cloudera.org:8080/8829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
Gerrit-Change-Number: 8829
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Comment-Date: Wed, 13 Dec 2017 13:38:35 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
Posted by "Kim Jin Chul (Code Review)" <ge...@cloudera.org>.
Kim Jin Chul has posted comments on this change. ( http://gerrit.cloudera.org:8080/8829 )
Change subject: IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8829/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8829/2//COMMIT_MSG@11
PS2, Line 11: ck.
> nit: long line
Done
http://gerrit.cloudera.org:8080/#/c/8829/2/be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp
File be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp:
http://gerrit.cloudera.org:8080/#/c/8829/2/be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp@37
PS2, Line 37: // To avoid of warning: 'PCG_USE_ZEROCHECK_ROTATE_IDIOM' is not defined, evaluates to 0
> Please wrap each in an ifndef block.
Done
http://gerrit.cloudera.org:8080/#/c/8829/2/be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp@599
PS2, Line 599: * warning: expansion of date or time macro is not reproducible
> nit: can you reword slightly - it's not just that clang-tidy complains, it'
I agree. It is better to show a main reason why disable the code.
--
To view, visit http://gerrit.cloudera.org:8080/8829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
Gerrit-Change-Number: 8829
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Comment-Date: Mon, 18 Dec 2017 05:14:00 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8829 )
Change subject: IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1635/
--
To view, visit http://gerrit.cloudera.org:8080/8829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
Gerrit-Change-Number: 8829
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Comment-Date: Mon, 18 Dec 2017 14:57:51 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/8829 )
Change subject: IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
Gerrit-Change-Number: 8829
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul <ji...@gmail.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Kim Jin Chul <ji...@gmail.com>
Gerrit-Comment-Date: Mon, 18 Dec 2017 14:57:46 +0000
Gerrit-HasComments: No