You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mahesh Reddy (Code Review)" <ge...@cloudera.org> on 2020/11/10 23:36:43 UTC
[kudu-CR] Change list to update include guards
Mahesh Reddy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16708
Change subject: Change list to update include guards
......................................................................
Change list to update include guards
This change list replaces the use of include guards to use '#pragma
once'. Header files have slowly been migrating to using this across the
Kudu codebase but this updates many files at once. None of the files
under 'gutil' or 'util' have been touched for now since they are exported
files.
Change-Id: I45aee1e9789389c9bf8c0c2080843552cd464237
---
M src/kudu/benchmarks/tpch/line_item_tsv_importer.h
M src/kudu/benchmarks/tpch/tpch-schemas.h
M src/kudu/cfile/binary_plain_block.h
M src/kudu/cfile/binary_prefix_block.h
M src/kudu/cfile/block_compression.h
M src/kudu/cfile/block_encodings.h
M src/kudu/cfile/block_handle.h
M src/kudu/cfile/block_pointer.h
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile_util.h
M src/kudu/cfile/index_block.h
M src/kudu/cfile/index_btree.h
M src/kudu/cfile/plain_bitmap_block.h
M src/kudu/cfile/plain_block.h
M src/kudu/cfile/rle_block.h
M src/kudu/cfile/type_encodings.h
M src/kudu/client/callbacks.h
M src/kudu/client/client_builder-internal.h
M src/kudu/client/columnar_scan_batch.h
M src/kudu/client/error-internal.h
M src/kudu/client/hash.h
M src/kudu/client/resource_metrics-internal.h
M src/kudu/client/resource_metrics.h
M src/kudu/client/row_result.h
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.h
M src/kudu/client/scanner-internal.h
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.h
M src/kudu/client/session-internal.h
M src/kudu/client/shared_ptr.h
M src/kudu/client/stubs.h
M src/kudu/client/table-internal.h
M src/kudu/client/tablet_server-internal.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.h
M src/kudu/client/write_op-internal.h
M src/kudu/client/write_op.h
M src/kudu/codegen/code_generator.h
M src/kudu/codegen/jit_wrapper.h
M src/kudu/codegen/module_builder.h
M src/kudu/codegen/precompiled.ll.h
M src/kudu/codegen/row_projector.h
M src/kudu/common/id_mapping.h
M src/kudu/common/iterator.h
M src/kudu/common/iterator_stats.h
M src/kudu/common/key_encoder.h
M src/kudu/common/partial_row.h
M src/kudu/common/row.h
M src/kudu/common/row_changelist.h
M src/kudu/common/rowid.h
M src/kudu/common/schema.h
M src/kudu/common/types.h
M src/kudu/common/wire_protocol-test-util.h
M src/kudu/common/wire_protocol.h
M src/kudu/consensus/log_anchor_registry.h
M src/kudu/consensus/log_cache.h
M src/kudu/consensus/log_metrics.h
M src/kudu/consensus/opid_util.h
M src/kudu/consensus/peer_manager.h
M src/kudu/fs/block_id.h
M src/kudu/fs/block_manager_metrics.h
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/fs-test-util.h
M src/kudu/master/master-test-util.h
M src/kudu/master/master_path_handlers.h
M src/kudu/master/table_metrics.h
M src/kudu/server/default_path_handlers.h
M src/kudu/server/generic_service.h
M src/kudu/server/monitored_task.h
M src/kudu/server/pprof_path_handlers.h
M src/kudu/server/rpcz-path-handler.h
M src/kudu/server/server_base_options.h
M src/kudu/server/tcmalloc_metrics.h
M src/kudu/server/tracing_path_handlers.h
M src/kudu/server/webserver_options.h
M src/kudu/tablet/cfile_set.h
M src/kudu/tablet/compaction_policy.h
M src/kudu/tablet/delta_applier.h
M src/kudu/tablet/delta_iterator_merger.h
M src/kudu/tablet/delta_stats.h
M src/kudu/tablet/lock_manager.h
M src/kudu/tablet/multi_column_writer.h
M src/kudu/tablet/mutation.h
M src/kudu/tablet/op_order_verifier.h
M src/kudu/tablet/rowset_info.h
M src/kudu/tablet/svg_dump.h
M src/kudu/tablet/tablet-test-base.h
M src/kudu/tablet/tablet_metrics.h
M src/kudu/tablet/tablet_replica_mm_ops.h
M src/kudu/tools/ksck_remote.h
M src/kudu/tserver/scanner_metrics.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_service.h
M src/kudu/tserver/tablet_replica_lookup.h
M src/kudu/tserver/tserver_path_handlers.h
96 files changed, 189 insertions(+), 443 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/16708/1
--
To view, visit http://gerrit.cloudera.org:8080/16708
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I45aee1e9789389c9bf8c0c2080843552cd464237
Gerrit-Change-Number: 16708
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] Change list to update include guards
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16708 )
Change subject: Change list to update include guards
......................................................................
Patch Set 2:
> I couldn't figure whether #pragma once is a C++11 feature. To be
> sure that we don't break compatibility with older compilers, all
> public C++ client headers like kudu/client/hash.h,
> columnar_scan_batch.h (mostly client header files without
> "-internal.h") shouldn't incorporate this change.
+1
Those external-facing API headers are supposed to be C++98 compliant. As for #pragma once, it's a non-standard, but widely supported feature:
* https://en.wikipedia.org/wiki/Pragma_once#Portability
* https://en.cppreference.com/w/cpp/preprocessor/imp
--
To view, visit http://gerrit.cloudera.org:8080/16708
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I45aee1e9789389c9bf8c0c2080843552cd464237
Gerrit-Change-Number: 16708
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 11 Nov 2020 03:46:20 +0000
Gerrit-HasComments: No
[kudu-CR] Change list to update include guards
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16708 )
Change subject: Change list to update include guards
......................................................................
Patch Set 2:
I couldn't figure whether #pragma once is a C++11 feature. To be sure that we don't break compatibility with older compilers, all public C++ client headers like kudu/client/hash.h, columnar_scan_batch.h (mostly client header files without "-internal.h") shouldn't incorporate this change.
--
To view, visit http://gerrit.cloudera.org:8080/16708
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I45aee1e9789389c9bf8c0c2080843552cd464237
Gerrit-Change-Number: 16708
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 11 Nov 2020 01:14:25 +0000
Gerrit-HasComments: No
[kudu-CR] Change list to update include guards
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16708
to look at the new patch set (#3).
Change subject: Change list to update include guards
......................................................................
Change list to update include guards
This change list replaces the use of include guards to use '#pragma
once'. Header files have slowly been migrating to using this across the
Kudu codebase but this updates many files at once. None of the files
under 'gutil' or 'util' have been touched for now since they are exported
files.
Change-Id: I45aee1e9789389c9bf8c0c2080843552cd464237
---
M src/kudu/benchmarks/tpch/line_item_tsv_importer.h
M src/kudu/benchmarks/tpch/tpch-schemas.h
M src/kudu/cfile/binary_plain_block.h
M src/kudu/cfile/binary_prefix_block.h
M src/kudu/cfile/block_compression.h
M src/kudu/cfile/block_encodings.h
M src/kudu/cfile/block_handle.h
M src/kudu/cfile/block_pointer.h
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile_util.h
M src/kudu/cfile/index_block.h
M src/kudu/cfile/index_btree.h
M src/kudu/cfile/plain_bitmap_block.h
M src/kudu/cfile/plain_block.h
M src/kudu/cfile/rle_block.h
M src/kudu/cfile/type_encodings.h
M src/kudu/client/callbacks.h
M src/kudu/client/client_builder-internal.h
M src/kudu/client/columnar_scan_batch.h
M src/kudu/client/error-internal.h
M src/kudu/client/hash.h
M src/kudu/client/resource_metrics-internal.h
M src/kudu/client/resource_metrics.h
M src/kudu/client/row_result.h
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.h
M src/kudu/client/scanner-internal.h
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.h
M src/kudu/client/session-internal.h
M src/kudu/client/shared_ptr.h
M src/kudu/client/stubs.h
M src/kudu/client/table-internal.h
M src/kudu/client/tablet_server-internal.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.h
M src/kudu/client/write_op-internal.h
M src/kudu/client/write_op.h
M src/kudu/codegen/code_generator.h
M src/kudu/codegen/jit_wrapper.h
M src/kudu/codegen/module_builder.h
M src/kudu/codegen/precompiled.ll.h
M src/kudu/codegen/row_projector.h
M src/kudu/common/id_mapping.h
M src/kudu/common/iterator.h
M src/kudu/common/iterator_stats.h
M src/kudu/common/key_encoder.h
M src/kudu/common/partial_row.h
M src/kudu/common/row.h
M src/kudu/common/row_changelist.h
M src/kudu/common/rowid.h
M src/kudu/common/schema.h
M src/kudu/common/types.h
M src/kudu/common/wire_protocol-test-util.h
M src/kudu/common/wire_protocol.h
M src/kudu/consensus/log_anchor_registry.h
M src/kudu/consensus/log_cache.h
M src/kudu/consensus/log_metrics.h
M src/kudu/consensus/opid_util.h
M src/kudu/consensus/peer_manager.h
M src/kudu/fs/block_id.h
M src/kudu/fs/block_manager_metrics.h
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/fs-test-util.h
M src/kudu/master/master-test-util.h
M src/kudu/master/master_path_handlers.h
M src/kudu/master/table_metrics.h
M src/kudu/server/default_path_handlers.h
M src/kudu/server/generic_service.h
M src/kudu/server/monitored_task.h
M src/kudu/server/pprof_path_handlers.h
M src/kudu/server/rpcz-path-handler.h
M src/kudu/server/server_base_options.h
M src/kudu/server/tcmalloc_metrics.h
M src/kudu/server/tracing_path_handlers.h
M src/kudu/server/webserver_options.h
M src/kudu/tablet/cfile_set.h
M src/kudu/tablet/compaction_policy.h
M src/kudu/tablet/delta_applier.h
M src/kudu/tablet/delta_iterator_merger.h
M src/kudu/tablet/delta_stats.h
M src/kudu/tablet/lock_manager.h
M src/kudu/tablet/multi_column_writer.h
M src/kudu/tablet/mutation.h
M src/kudu/tablet/op_order_verifier.h
M src/kudu/tablet/rowset_info.h
M src/kudu/tablet/svg_dump.h
M src/kudu/tablet/tablet-test-base.h
M src/kudu/tablet/tablet_metrics.h
M src/kudu/tablet/tablet_replica_mm_ops.h
M src/kudu/tools/ksck_remote.h
M src/kudu/tserver/scanner_metrics.h
M src/kudu/tserver/tablet_copy-test-base.h
M src/kudu/tserver/tablet_copy_service.h
M src/kudu/tserver/tablet_replica_lookup.h
M src/kudu/tserver/tserver_path_handlers.h
96 files changed, 187 insertions(+), 435 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/16708/3
--
To view, visit http://gerrit.cloudera.org:8080/16708
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I45aee1e9789389c9bf8c0c2080843552cd464237
Gerrit-Change-Number: 16708
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] Change list to update include guards
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has abandoned this change. ( http://gerrit.cloudera.org:8080/16708 )
Change subject: Change list to update include guards
......................................................................
Abandoned
--
To view, visit http://gerrit.cloudera.org:8080/16708
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I45aee1e9789389c9bf8c0c2080843552cd464237
Gerrit-Change-Number: 16708
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] Change list to update include guards
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16708 )
Change subject: Change list to update include guards
......................................................................
Patch Set 3:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/16708/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16708/3//COMMIT_MSG@11
PS3, Line 11: None of the files
: under 'gutil' or 'util' have been touched for now since they are exported
: files.
As I understand, the only exported files in 'util' are:
int128.h
kudu_export.h
monotime.h
slice.h
status.h
And none of files under 'gutil' are exported.
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/callbacks.h
File src/kudu/client/callbacks.h:
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/callbacks.h@17
PS3, Line 17: #pragma once
Since 'pragma once' isn't a standard feature (even if supported by most contemporary C++ compilers: https://en.wikipedia.org/wiki/Pragma_once#Portability), I think it's better to keep the original include guard here because this file is targeted for external consumption by old c++98 compilers (like gcc 2.95 and other pre-3.4.2 gcc releaes) as well.
Overall, a good criteria to determine whether the file is for external consumption is to look for KUDU_EXPORT macro and doxygen-style comments. A better option is to check what header files are involved in 'kudu_client_exported' target from src/kudu/client/CMakefile
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/columnar_scan_batch.h
File src/kudu/client/columnar_scan_batch.h:
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/columnar_scan_batch.h@17
PS3, Line 17: #pragma once
ditto
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/hash.h
File src/kudu/client/hash.h:
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/hash.h@17
PS3, Line 17: #pragma once
ditto
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/resource_metrics.h
File src/kudu/client/resource_metrics.h:
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/resource_metrics.h@17
PS3, Line 17: #pragma once
ditto
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/row_result.h
File src/kudu/client/row_result.h:
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/row_result.h@17
PS3, Line 17: #pragma once
ditto
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/scan_batch.h
File src/kudu/client/scan_batch.h:
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/scan_batch.h@17
PS3, Line 17: #pragma once
ditto
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/scan_predicate.h
File src/kudu/client/scan_predicate.h:
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/scan_predicate.h@17
PS3, Line 17: #pragma once
ditto
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/schema.h@18
PS3, Line 18: #pragma once
dito
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/shared_ptr.h
File src/kudu/client/shared_ptr.h:
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/shared_ptr.h@18
PS3, Line 18: #pragma once
ditto
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/stubs.h
File src/kudu/client/stubs.h:
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/stubs.h@17
PS3, Line 17: #pragma once
ditto
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/value.h
File src/kudu/client/value.h:
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/value.h@17
PS3, Line 17: #pragma once
ditto
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/write_op.h
File src/kudu/client/write_op.h:
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/client/write_op.h@17
PS3, Line 17: #pragma once
ditto
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/common/partial_row.h
File src/kudu/common/partial_row.h:
http://gerrit.cloudera.org:8080/#/c/16708/3/src/kudu/common/partial_row.h@17
PS3, Line 17: #pragma once
ditto
--
To view, visit http://gerrit.cloudera.org:8080/16708
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I45aee1e9789389c9bf8c0c2080843552cd464237
Gerrit-Change-Number: 16708
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 26 Nov 2020 00:53:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] Change list to update include guards
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16708 )
Change subject: Change list to update include guards
......................................................................
Patch Set 3:
> Patch Set 2:
>
> > I couldn't figure whether #pragma once is a C++11 feature. To be
> > sure that we don't break compatibility with older compilers, all
> > public C++ client headers like kudu/client/hash.h,
> > columnar_scan_batch.h (mostly client header files without
> > "-internal.h") shouldn't incorporate this change.
>
> +1
> Those external-facing API headers are supposed to be C++98 compliant. As for #pragma once, it's a non-standard, but widely supported feature:
>
> * https://en.wikipedia.org/wiki/Pragma_once#Portability
> * https://en.cppreference.com/w/cpp/preprocessor/imp
Sorry was busy rebasing this branch to avoid merge conflicts but I will revert the changes for the necessary header files.
--
To view, visit http://gerrit.cloudera.org:8080/16708
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I45aee1e9789389c9bf8c0c2080843552cd464237
Gerrit-Change-Number: 16708
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 11 Nov 2020 06:26:26 +0000
Gerrit-HasComments: No