You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dinesh Bhat (Code Review)" <ge...@cloudera.org> on 2016/09/22 21:25:27 UTC
[kudu-CR] [misc] : Remove few more warnings
Hello Dan Burkert, Adar Dembo, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4518
to review the following change.
Change subject: [misc] : Remove few more warnings
......................................................................
[misc] : Remove few more warnings
Observed few warnings with fresh builds:
[365/1113] Building CXX object src/kudu/common/CMakeFiles/kudu_common_exported.dir/row_operations.cc.o
../../src/kudu/common/row_operations.cc: In member function \u2018std::string kudu::DecodedRowOperation::ToString(const kudu::Schema&) const\u2019:
../../src/kudu/common/row_operations.cc:58:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
[403/1113] Building CXX object src/kudu/common/CMakeFiles/kudu_common.dir/row_operations.cc.o
../../src/kudu/common/row_operations.cc: In member function \u2018std::string kudu::DecodedRowOperation::ToString(const kudu::Schema&) const\u2019:
../../src/kudu/common/row_operations.cc:58:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
[516/1113] Building CXX object src/kudu/benchmarks/CMakeFiles/rle.dir/rle.cc.o
../../src/kudu/benchmarks/rle.cc: In function \u2018void kudu::BooleanRLE()\u2019:
../../src/kudu/benchmarks/rle.cc:93:10: warning: variable \u2018run_length\u2019 set but not used [-Wunused-but-set-variable]
size_t run_length;
^
[575/1113] Building CXX object src/kudu/cfile/CMakeFiles/cfile.dir/compression_codec.cc.o
In file included from ../../src/kudu/cfile/compression_codec.h:24:0,
from ../../src/kudu/cfile/compression_codec.cc:26:
../../src/kudu/gutil/macros.h:102:0: warning: "DISALLOW_COPY_AND_ASSIGN" redefined
#define DISALLOW_COPY_AND_ASSIGN(TypeName) \
^
In file included from ../../thirdparty/installed-deps/include/snappy.h:45:0,
from ../../src/kudu/cfile/compression_codec.cc:20:
../../thirdparty/installed-deps/include/snappy-stubs-public.h:79:0: note: this is the location of the previous definition
#define DISALLOW_COPY_AND_ASSIGN(TypeName) \
^
Also added a trivial comment update.
Change-Id: I89b96d52dfed6b38f17cf8cdebeed840fb32f98d
---
M src/kudu/benchmarks/rle.cc
M src/kudu/codegen/row_projector.cc
M src/kudu/common/row_operations.cc
M src/kudu/gutil/macros.h
4 files changed, 6 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/4518/1
--
To view, visit http://gerrit.cloudera.org:8080/4518
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I89b96d52dfed6b38f17cf8cdebeed840fb32f98d
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
[kudu-CR] [misc] : Remove few more warnings
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [misc] : Remove few more warnings
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4518/1/src/kudu/benchmarks/rle.cc
File src/kudu/benchmarks/rle.cc:
PS1, Line 94: (void)
Why is the cast on the return value necessary if it's not being assigned anywhere?
--
To view, visit http://gerrit.cloudera.org:8080/4518
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89b96d52dfed6b38f17cf8cdebeed840fb32f98d
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [misc] : Remove few more warnings
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [misc] : Remove few more warnings
......................................................................
Patch Set 2: Verified+1
I verified the patch, building in release and debug configurations at ve0518.halxg.cloudera.com.
--
To view, visit http://gerrit.cloudera.org:8080/4518
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89b96d52dfed6b38f17cf8cdebeed840fb32f98d
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [misc] : Remove few more warnings
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged.
Change subject: [misc] : Remove few more warnings
......................................................................
[misc] : Remove few more warnings
Observed few warnings with fresh builds:
[365/1113] Building CXX object src/kudu/common/CMakeFiles/kudu_common_exported.dir/row_operations.cc.o
../../src/kudu/common/row_operations.cc: In member function \u2018std::string kudu::DecodedRowOperation::ToString(const kudu::Schema&) const\u2019:
../../src/kudu/common/row_operations.cc:58:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
[403/1113] Building CXX object src/kudu/common/CMakeFiles/kudu_common.dir/row_operations.cc.o
../../src/kudu/common/row_operations.cc: In member function \u2018std::string kudu::DecodedRowOperation::ToString(const kudu::Schema&) const\u2019:
../../src/kudu/common/row_operations.cc:58:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
[516/1113] Building CXX object src/kudu/benchmarks/CMakeFiles/rle.dir/rle.cc.o
../../src/kudu/benchmarks/rle.cc: In function \u2018void kudu::BooleanRLE()\u2019:
../../src/kudu/benchmarks/rle.cc:93:10: warning: variable \u2018run_length\u2019 set but not used [-Wunused-but-set-variable]
size_t run_length;
^
[575/1113] Building CXX object src/kudu/cfile/CMakeFiles/cfile.dir/compression_codec.cc.o
In file included from ../../src/kudu/cfile/compression_codec.h:24:0,
from ../../src/kudu/cfile/compression_codec.cc:26:
../../src/kudu/gutil/macros.h:102:0: warning: "DISALLOW_COPY_AND_ASSIGN" redefined
#define DISALLOW_COPY_AND_ASSIGN(TypeName) \
^
In file included from ../../thirdparty/installed-deps/include/snappy.h:45:0,
from ../../src/kudu/cfile/compression_codec.cc:20:
../../thirdparty/installed-deps/include/snappy-stubs-public.h:79:0: note: this is the location of the previous definition
#define DISALLOW_COPY_AND_ASSIGN(TypeName) \
^
Also added a trivial comment update.
Change-Id: I89b96d52dfed6b38f17cf8cdebeed840fb32f98d
Reviewed-on: http://gerrit.cloudera.org:8080/4518
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/benchmarks/rle.cc
M src/kudu/codegen/row_projector.cc
M src/kudu/common/row_operations.cc
M src/kudu/gutil/macros.h
4 files changed, 7 insertions(+), 10 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Alexey Serbin: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4518
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I89b96d52dfed6b38f17cf8cdebeed840fb32f98d
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
[kudu-CR] [misc] : Remove few more warnings
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: [misc] : Remove few more warnings
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4518/1/src/kudu/benchmarks/rle.cc
File src/kudu/benchmarks/rle.cc:
PS1, Line 94: r (int
> Ah, I see. We typically use ignore_result() (see gutil/basictypes.h) for th
yep, that works, thanks, updated the patch.
--
To view, visit http://gerrit.cloudera.org:8080/4518
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89b96d52dfed6b38f17cf8cdebeed840fb32f98d
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [misc] : Remove few more warnings
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [misc] : Remove few more warnings
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4518
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89b96d52dfed6b38f17cf8cdebeed840fb32f98d
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [misc] : Remove few more warnings
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Adar Dembo, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4518
to look at the new patch set (#2).
Change subject: [misc] : Remove few more warnings
......................................................................
[misc] : Remove few more warnings
Observed few warnings with fresh builds:
[365/1113] Building CXX object src/kudu/common/CMakeFiles/kudu_common_exported.dir/row_operations.cc.o
../../src/kudu/common/row_operations.cc: In member function \u2018std::string kudu::DecodedRowOperation::ToString(const kudu::Schema&) const\u2019:
../../src/kudu/common/row_operations.cc:58:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
[403/1113] Building CXX object src/kudu/common/CMakeFiles/kudu_common.dir/row_operations.cc.o
../../src/kudu/common/row_operations.cc: In member function \u2018std::string kudu::DecodedRowOperation::ToString(const kudu::Schema&) const\u2019:
../../src/kudu/common/row_operations.cc:58:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
[516/1113] Building CXX object src/kudu/benchmarks/CMakeFiles/rle.dir/rle.cc.o
../../src/kudu/benchmarks/rle.cc: In function \u2018void kudu::BooleanRLE()\u2019:
../../src/kudu/benchmarks/rle.cc:93:10: warning: variable \u2018run_length\u2019 set but not used [-Wunused-but-set-variable]
size_t run_length;
^
[575/1113] Building CXX object src/kudu/cfile/CMakeFiles/cfile.dir/compression_codec.cc.o
In file included from ../../src/kudu/cfile/compression_codec.h:24:0,
from ../../src/kudu/cfile/compression_codec.cc:26:
../../src/kudu/gutil/macros.h:102:0: warning: "DISALLOW_COPY_AND_ASSIGN" redefined
#define DISALLOW_COPY_AND_ASSIGN(TypeName) \
^
In file included from ../../thirdparty/installed-deps/include/snappy.h:45:0,
from ../../src/kudu/cfile/compression_codec.cc:20:
../../thirdparty/installed-deps/include/snappy-stubs-public.h:79:0: note: this is the location of the previous definition
#define DISALLOW_COPY_AND_ASSIGN(TypeName) \
^
Also added a trivial comment update.
Change-Id: I89b96d52dfed6b38f17cf8cdebeed840fb32f98d
---
M src/kudu/benchmarks/rle.cc
M src/kudu/codegen/row_projector.cc
M src/kudu/common/row_operations.cc
M src/kudu/gutil/macros.h
4 files changed, 7 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/4518/2
--
To view, visit http://gerrit.cloudera.org:8080/4518
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I89b96d52dfed6b38f17cf8cdebeed840fb32f98d
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [misc] : Remove few more warnings
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: [misc] : Remove few more warnings
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4518/1/src/kudu/benchmarks/rle.cc
File src/kudu/benchmarks/rle.cc:
PS1, Line 94: (void)
> Why is the cast on the return value necessary if it's not being assigned an
Well, if anything it was only for making it explicitly clear to reader that the routine is returning something and the caller ignores it. Just saw the google cpp guideline, perhaps I could have been C++ish here with static_cast than C-style (void) ? Or shall I remove this altogether to keep consistency with other places ?
--
To view, visit http://gerrit.cloudera.org:8080/4518
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89b96d52dfed6b38f17cf8cdebeed840fb32f98d
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [misc] : Remove few more warnings
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [misc] : Remove few more warnings
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4518/1/src/kudu/benchmarks/rle.cc
File src/kudu/benchmarks/rle.cc:
PS1, Line 94: (void)
> Well, if anything it was only for making it explicitly clear to reader that
Ah, I see. We typically use ignore_result() (see gutil/basictypes.h) for that purpose.
--
To view, visit http://gerrit.cloudera.org:8080/4518
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89b96d52dfed6b38f17cf8cdebeed840fb32f98d
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes