You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Anonymous Coward (Code Review)" <ge...@cloudera.org> on 2023/08/31 20:13:29 UTC
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
gaurav1086@gmail.com has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20445
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
This enables the clang tidy performance check:
performance-inefficient-string-concatenation
"warning: string concatenation results in allocation of unnecessary
temporary strings"
Fix: Use StrCat() to concatenate multiple strings
Testing:
- Ran bin/run_clang_tidy.sh with the new checks
- Ran GVO
Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
---
M be/src/exprs/expr-test.cc
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/timestamp-test.cc
M be/src/scheduling/cluster-membership-test-util.cc
M be/src/util/runtime-profile.cc
5 files changed, 82 insertions(+), 76 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/20445/1
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <ga...@gmail.com>
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 8: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 8
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Sep 2023 02:48:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20445/5/.clang-tidy
File .clang-tidy:
http://gerrit.cloudera.org:8080/#/c/20445/5/.clang-tidy@78
PS5, Line 78: performance-inefficient-string-concatenation,\
> Let's just go ahead and delete this line and let the performance-* take car
What I mean is that the "-performance-inefficient-string-concatenation" line which disables the check should be removed.
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 5
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Sep 2023 19:06:44 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 10:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9729/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 10
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 19 Sep 2023 04:32:12 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 1:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/20445/1/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
http://gerrit.cloudera.org:8080/#/c/20445/1/be/src/exprs/expr-test.cc@906
PS1, Line 906: TestValue(operand + ' ' + operators[i] + StrCat(" " + operand), TYPE_BOOLEAN, is_equal);
line too long (96 > 90)
http://gerrit.cloudera.org:8080/#/c/20445/1/be/src/exprs/expr-test.cc@4997
PS1, Line 4997: TestValue(StrCat("length(base64encode('", as_octal, "')) > length('", as_octal, "')"),
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/20445/1/be/src/exprs/expr-test.cc@4999
PS1, Line 4999: TestValue(StrCat("base64decode(base64encode('", as_octal + "')) = '", as_octal, "'"),
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/20445/1/be/src/exprs/expr-test.cc@6050
PS1, Line 6050: TestStringValue(StrCat("conv(", q, lexical_cast<string>(numeric_limits<int64_t>::max())
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/20445/1/be/src/exprs/expr-test.cc@6052
PS1, Line 6052: TestStringValue(StrCat("conv(", q, lexical_cast<string>(numeric_limits<int64_t>::max())
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/20445/1/be/src/exprs/expr-test.cc@6054
PS1, Line 6054: TestStringValue(StrCat("conv(", q, lexical_cast<string>(numeric_limits<int64_t>::min()+1)
line too long (93 > 90)
http://gerrit.cloudera.org:8080/#/c/20445/1/be/src/exprs/expr-test.cc@6056
PS1, Line 6056: TestStringValue(StrCat("conv(", q, lexical_cast<string>(numeric_limits<int64_t>::min()+1)
line too long (93 > 90)
http://gerrit.cloudera.org:8080/#/c/20445/1/be/src/exprs/expr-test.cc@6946
PS1, Line 6946: TestIsNull(StrCat(unit, "_add(years_add(cast('9999-12-31 23:59:59' as timestamp), 1), "
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/20445/1/be/src/exprs/expr-test.cc@6948
PS1, Line 6948: TestIsNull(StrCat(unit, "_sub(cast('1400-01-01 00:00:00' as timestamp), ", max_int, ")"),
line too long (93 > 90)
http://gerrit.cloudera.org:8080/#/c/20445/1/be/src/exprs/expr-test.cc@6950
PS1, Line 6950: TestIsNull(StrCat(unit, "_add(years_add(cast('9999-12-31 23:59:59' as timestamp), 1), "
line too long (91 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <ga...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Aug 2023 20:14:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 4:
Now that https://gerrit.cloudera.org/#/c/20387/ has merged, please rebase on top of it. Then also do the .clang-tidy change that Michael mentioned to enable this by default.
There is one usability thing that I would like to add to this change. Since we are mostly solving this using StrCat(), it would be nice for our clang-tidy output to mention it if performance-inefficient-string-concatenation is mentioned.
I would preserve the original Clang message and append an Impala specific note saying that Impala code can also solve the issue using the StrCat() function from be/src/gutil/strings/strcat.h.
One way to do that is to pipe output through this:
sed '/performance-inefficient-string-concatenation/ s#$# (Impala-specific note: This can also be fixed using the StrCat() function from be/src/gutil/strings/strcat.h)#'
It matches the performance-inefficient-string-concatenation lines and replaces the end of line with our message.
We'll have to figure out the right part of bin/run_clang_tidy.sh to modify.
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 Sep 2023 16:58:02 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13931/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Sep 2023 16:12:02 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 8:
(1 comment)
This looks good, one small nit then we are good.
http://gerrit.cloudera.org:8080/#/c/20445/8/bin/run_clang_tidy.sh
File bin/run_clang_tidy.sh:
http://gerrit.cloudera.org:8080/#/c/20445/8/bin/run_clang_tidy.sh@62
PS8, Line 62: STRCAT_MESSAGE="Impala-specific note:This can also be fixed using the StrCat() function\
: from be/src/gutil/strings strcat.h)"
One small nit: Add a space after "Impala-specific note:" (before "This") and another after "function" before the \
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 8
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Mon, 18 Sep 2023 21:25:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 9:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9726/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 9
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Mon, 18 Sep 2023 22:41:39 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
gsingh@cloudera.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 7:
> Uploaded patch set 6.
Moved the sed command as the last filter.
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 7
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Sep 2023 20:22:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 9: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/9726/
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 9
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 19 Sep 2023 03:16:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13907/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Aug 2023 22:56:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20445/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20445/4//COMMIT_MSG@10
PS4, Line 10: performance-inefficient-string-concatenation
This looks like it requires a change to .clang-tidy. Presumably it should wait until after https://gerrit.cloudera.org/c/20387/ is merged.
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Sep 2023 16:24:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Sep 2023 01:41:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 10: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 10
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 19 Sep 2023 04:32:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
gsingh@cloudera.com has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
This enables the clang tidy performance check:
performance-inefficient-string-concatenation
"warning: string concatenation results in allocation of unnecessary
temporary strings"
Fix: Use StrCat() to concatenate multiple strings
Testing:
- Ran bin/run_clang_tidy.sh with the new checks
- Ran GVO
Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
---
M be/src/exprs/expr-test.cc
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/timestamp-test.cc
M be/src/scheduling/cluster-membership-test-util.cc
M be/src/util/runtime-profile.cc
5 files changed, 88 insertions(+), 79 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/20445/2
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
gsingh@cloudera.com has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
This enables the clang tidy performance check:
performance-inefficient-string-concatenation
"warning: string concatenation results in allocation of unnecessary
temporary strings"
Fix: Use StrCat() to concatenate multiple strings
Testing:
- Ran bin/run_clang_tidy.sh with the new checks
- Ran GVO
Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
---
M be/src/exprs/expr-test.cc
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/timestamp-test.cc
M be/src/scheduling/cluster-membership-test-util.cc
M be/src/util/runtime-profile.cc
5 files changed, 88 insertions(+), 79 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/20445/3
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
gsingh@cloudera.com has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
This enables the clang tidy performance check:
performance-inefficient-string-concatenation
"warning: string concatenation results in allocation of unnecessary
temporary strings"
Fix: Use StrCat() to concatenate multiple strings
Testing:
- Ran bin/run_clang_tidy.sh with the new checks
- Ran GVO
Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
---
M .clang-tidy
M be/src/exprs/expr-test.cc
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/timestamp-test.cc
M be/src/scheduling/cluster-membership-test-util.cc
M be/src/util/runtime-profile.cc
M bin/run_clang_tidy.sh
7 files changed, 93 insertions(+), 81 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/20445/8
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 8
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20445/5/.clang-tidy
File .clang-tidy:
http://gerrit.cloudera.org:8080/#/c/20445/5/.clang-tidy@78
PS5, Line 78: performance-inefficient-string-concatenation,\
Let's just go ahead and delete this line and let the performance-* take care of this.
http://gerrit.cloudera.org:8080/#/c/20445/5/bin/run_clang_tidy.sh
File bin/run_clang_tidy.sh:
http://gerrit.cloudera.org:8080/#/c/20445/5/bin/run_clang_tidy.sh@65
PS5, Line 65: sed '/performance-inefficient-string-concatenation/ s#$# (Impala-specific note:\
: This can also be fixed using the StrCat() function from be/src/gutil/strings/\
: strcat.h)#' 2> ${TMP_STDERR};
Comment 1:
The output that we care about goes to stdout, but run-clang-tidy.py also produces messages that go to stderr. The pipe only forwards stdout, so if we have this:
A | B 2> ${OUT_STDERR}
None of the stderr output from A actually gets to ${OUT_STDERR}.
It looks like we need:
A 2> ${OUT_STDERR} | B
The stderr in this case is mostly garbage, but we like to print it if run-clang-tidy.py fails.
Comment 2:
Bash doesn't know to remove the whitespace due to indentation, so our message will include the spaces. To avoid that, one option is to define a local variable like this:
STRCAT_MESSAGE="(Impala-specific note: This can also be fixed using the StrCat() \
function from be/src/gutil/strings/strcat.h)"
The important piece is that after the "\" the next line has no indentation.
To include the variable in our sed command, we have to use double quotes rather than single quotes, which requires some other changes to avoid using characters that terminate the sed pattern. Here is something that works for me (changing the #'s to ='s):
sed "/performance-inefficient-string-concatenation/ s=$= ${STRCAT_MESSAGE}="
The alternative is to define a variable with the whole pattern using single quotes (and avoiding indentation) and then do sed ${STRCAT_MESSAGE_PATTERN}.
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 5
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Sep 2023 16:42:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
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/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
This enables the clang tidy performance check:
performance-inefficient-string-concatenation
"warning: string concatenation results in allocation of unnecessary
temporary strings"
Fix: Use StrCat() to concatenate multiple strings
Testing:
- Ran bin/run_clang_tidy.sh with the new checks
- Ran GVO
Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Reviewed-on: http://gerrit.cloudera.org:8080/20445
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M .clang-tidy
M be/src/exprs/expr-test.cc
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/timestamp-test.cc
M be/src/scheduling/cluster-membership-test-util.cc
M be/src/util/runtime-profile.cc
M bin/run_clang_tidy.sh
7 files changed, 93 insertions(+), 81 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 11
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 9:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14019/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 9
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Mon, 18 Sep 2023 23:06:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
gsingh@cloudera.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 8:
> (2 comments)
Both comments addressed in the latest patch.
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 8
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Sep 2023 21:23:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
gsingh@cloudera.com has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
This enables the clang tidy performance check:
performance-inefficient-string-concatenation
"warning: string concatenation results in allocation of unnecessary
temporary strings"
Fix: Use StrCat() to concatenate multiple strings
Testing:
- Ran bin/run_clang_tidy.sh with the new checks
- Ran GVO
Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
---
M .clang-tidy
M be/src/exprs/expr-test.cc
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/timestamp-test.cc
M be/src/scheduling/cluster-membership-test-util.cc
M be/src/util/runtime-profile.cc
M bin/run_clang_tidy.sh
7 files changed, 93 insertions(+), 81 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/20445/5
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 5
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
gsingh@cloudera.com has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 7:
> (1 comment)
Removed in the latest patch.
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 7
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Sep 2023 20:47:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 8:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13994/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 8
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Sep 2023 21:50:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9679/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Sep 2023 21:31:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9666/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Aug 2023 23:43:12 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20445/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
http://gerrit.cloudera.org:8080/#/c/20445/2/be/src/exprs/expr-test.cc@6051
PS2, Line 6051: TestStringValue(StrCat("conv(", q,
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/20445/2/be/src/exprs/expr-test.cc@6052
PS2, Line 6052: lexical_cast<string>(numeric_limits<int64_t>::max()), q, ", 36, 16)"),
line has trailing whitespace
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Aug 2023 22:28:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 8: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 8
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Sep 2023 18:06:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 10: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 10
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 19 Sep 2023 08:55:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has uploaded a new patch set (#9) to the change originally created by gsingh@cloudera.com. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
This enables the clang tidy performance check:
performance-inefficient-string-concatenation
"warning: string concatenation results in allocation of unnecessary
temporary strings"
Fix: Use StrCat() to concatenate multiple strings
Testing:
- Ran bin/run_clang_tidy.sh with the new checks
- Ran GVO
Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
---
M .clang-tidy
M be/src/exprs/expr-test.cc
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/timestamp-test.cc
M be/src/scheduling/cluster-membership-test-util.cc
M be/src/util/runtime-profile.cc
M bin/run_clang_tidy.sh
7 files changed, 93 insertions(+), 81 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/20445/9
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 9
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13993/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 7
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Sep 2023 20:47:10 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13904/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Aug 2023 20:40:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/13906/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 2
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Aug 2023 22:53:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 3: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/9666/
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 3
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 01 Sep 2023 04:01:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 9: Code-Review+2
Uploaded a new patch to fix my nit
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 9
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Mon, 18 Sep 2023 22:41:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
gsingh@cloudera.com has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
This enables the clang tidy performance check:
performance-inefficient-string-concatenation
"warning: string concatenation results in allocation of unnecessary
temporary strings"
Fix: Use StrCat() to concatenate multiple strings
Testing:
- Ran bin/run_clang_tidy.sh with the new checks
- Ran GVO
Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
---
M .clang-tidy
M be/src/exprs/expr-test.cc
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/timestamp-test.cc
M be/src/scheduling/cluster-membership-test-util.cc
M be/src/util/runtime-profile.cc
M bin/run_clang_tidy.sh
7 files changed, 92 insertions(+), 81 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/20445/7
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 7
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 8:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9710/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 8
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Sep 2023 22:18:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
Patch Set 5:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/13978/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 5
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 Sep 2023 23:20:41 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
gsingh@cloudera.com has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/20445 )
Change subject: IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
......................................................................
IMPALA-12390 (part 2): Enable some clang-tidy performance related checks
This enables the clang tidy performance check:
performance-inefficient-string-concatenation
"warning: string concatenation results in allocation of unnecessary
temporary strings"
Fix: Use StrCat() to concatenate multiple strings
Testing:
- Ran bin/run_clang_tidy.sh with the new checks
- Ran GVO
Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
---
M be/src/exprs/expr-test.cc
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/timestamp-test.cc
M be/src/scheduling/cluster-membership-test-util.cc
M be/src/util/runtime-profile.cc
5 files changed, 88 insertions(+), 79 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/20445/4
--
To view, visit http://gerrit.cloudera.org:8080/20445
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibad8bd0f12aab92ad874f5a6b9ec922dce7f3190
Gerrit-Change-Number: 20445
Gerrit-PatchSet: 4
Gerrit-Owner: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>