[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

2017-12-18 Thread Kim Jin Chul (Code Review) 
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 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 19 Dec 2017 00:03:12 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

2017-12-18 Thread Impala Public Jenkins (Code Review) 
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 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Mon, 18 Dec 2017 18:37:44 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

2017-12-18 Thread Impala Public Jenkins (Code Review) 
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 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Mon, 18 Dec 2017 14:57:51 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

2017-12-17 Thread Kim Jin Chul (Code Review) 
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 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Mon, 18 Dec 2017 05:14:00 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

2017-12-17 Thread Kim Jin Chul (Code Review) 
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 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul

[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

2017-12-17 Thread Jim Apple (Code Review) 
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 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Mon, 18 Dec 2017 04:19:20 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

2017-12-13 Thread Kim Jin Chul (Code Review) 
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 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Wed, 13 Dec 2017 13:38:35 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

2017-12-13 Thread Kim Jin Chul (Code Review) 
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 
Gerrit-Reviewer: Jim Apple

[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

2017-12-13 Thread Kim Jin Chul (Code Review) 
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