You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2017/08/29 00:47:53 UTC

[kudu-CR] [iwyu] fix on 

Alexey Serbin has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/7867

Change subject: [iwyu] fix on <ext/alloc_traits.h>
......................................................................

[iwyu] fix on <ext/alloc_traits.h>

Fixed the issue where IWYU suggested to include the internal GCC header
<ext/alloc_traits.h>: added additional mapping file which declares the
header as a private one, suggesting to include one of the public header
files of the standard C++ library files which include the private header
file itself and has other references in the context.

Change-Id: I3f4b517e9816294d70c57110182132adc6cd46d9
---
M build-support/iwyu/iwyu-filter.awk
A build-support/iwyu/mappings/libstdcpp.imp
M src/kudu/cfile/index_btree.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/common/schema.cc
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/log_cache.cc
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_util.cc
M src/kudu/experiments/merge-test.cc
M src/kudu/gutil/strings/split.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/integration-tests/fuzz-itest.cc
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/master-test.cc
M src/kudu/security/token-test.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/lock_manager-test.cc
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset_tree-test.cc
M src/kudu/tablet/rowset_tree.cc
M src/kudu/tablet/tablet-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tserver/scanners-test.cc
M src/kudu/util/env-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/make_shared.h
M src/kudu/util/mt-threadlocal-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/threadpool-test.cc
39 files changed, 17 insertions(+), 40 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/7867/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7867
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3f4b517e9816294d70c57110182132adc6cd46d9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>

[kudu-CR] [iwyu] fix on

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/7867

to look at the new patch set (#3).

Change subject: [iwyu] fix on <ext/alloc_traits.h>
......................................................................

[iwyu] fix on <ext/alloc_traits.h>

Fixed the issue where IWYU suggested to include the internal GCC header
<ext/alloc_traits.h>: added additional mapping file which declares the
header as a private one, suggesting to include one of the public header
files of the standard C++ library files which include the private header
file itself and has other references in the context.

Change-Id: I3f4b517e9816294d70c57110182132adc6cd46d9
---
M build-support/iwyu/iwyu-filter.awk
M build-support/iwyu/iwyu.sh
A build-support/iwyu/mappings/libstdcpp.imp
M src/kudu/cfile/index_btree.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/common/schema.cc
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/log_cache.cc
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_util.cc
M src/kudu/experiments/merge-test.cc
M src/kudu/gutil/strings/split.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/integration-tests/fuzz-itest.cc
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/master-test.cc
M src/kudu/security/token-test.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/lock_manager-test.cc
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset_tree-test.cc
M src/kudu/tablet/rowset_tree.cc
M src/kudu/tablet/tablet-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tserver/scanners-test.cc
M src/kudu/util/env-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/make_shared.h
M src/kudu/util/mt-threadlocal-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/threadpool-test.cc
40 files changed, 35 insertions(+), 41 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/7867/3
-- 
To view, visit http://gerrit.cloudera.org:8080/7867
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3f4b517e9816294d70c57110182132adc6cd46d9
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [iwyu] fix on

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/7867

to look at the new patch set (#4).

Change subject: [iwyu] fix on <ext/alloc_traits.h>
......................................................................

[iwyu] fix on <ext/alloc_traits.h>

Fixed the issue where IWYU suggested to include the internal GCC header
<ext/alloc_traits.h>: added additional mapping file which declares the
header as a private one, suggesting to include one of the public header
files of the standard C++ library files which include the private header
file itself and has other references in the context.

Change-Id: I3f4b517e9816294d70c57110182132adc6cd46d9
---
M build-support/iwyu/iwyu-filter.awk
M build-support/iwyu/iwyu.sh
A build-support/iwyu/mappings/libstdcpp.imp
M src/kudu/cfile/index_btree.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/common/schema.cc
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/log_cache.cc
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_util.cc
M src/kudu/experiments/merge-test.cc
M src/kudu/gutil/strings/split.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/integration-tests/fuzz-itest.cc
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/master-test.cc
M src/kudu/security/token-test.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/lock_manager-test.cc
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset_tree-test.cc
M src/kudu/tablet/rowset_tree.cc
M src/kudu/tablet/tablet-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tserver/scanners-test.cc
M src/kudu/util/env-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/make_shared.h
M src/kudu/util/mt-threadlocal-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/threadpool-test.cc
40 files changed, 37 insertions(+), 43 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/7867/4
-- 
To view, visit http://gerrit.cloudera.org:8080/7867
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3f4b517e9816294d70c57110182132adc6cd46d9
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] [iwyu] fix on

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has uploaded a new patch set (#2).

Change subject: [iwyu] fix on <ext/alloc_traits.h>
......................................................................

[iwyu] fix on <ext/alloc_traits.h>

Fixed the issue where IWYU suggested to include the internal GCC header
<ext/alloc_traits.h>: added additional mapping file which declares the
header as a private one, suggesting to include one of the public header
files of the standard C++ library files which include the private header
file itself and has other references in the context.

Change-Id: I3f4b517e9816294d70c57110182132adc6cd46d9
---
M build-support/iwyu/iwyu-filter.awk
A build-support/iwyu/mappings/libstdcpp.imp
M src/kudu/cfile/index_btree.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/common/schema.cc
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/log_cache.cc
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_util.cc
M src/kudu/experiments/merge-test.cc
M src/kudu/gutil/strings/split.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/integration-tests/fuzz-itest.cc
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/master-test.cc
M src/kudu/security/token-test.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/lock_manager-test.cc
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset_tree-test.cc
M src/kudu/tablet/rowset_tree.cc
M src/kudu/tablet/tablet-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tserver/scanners-test.cc
M src/kudu/util/env-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/make_shared.h
M src/kudu/util/mt-threadlocal-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/threadpool-test.cc
39 files changed, 33 insertions(+), 40 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/7867/2
-- 
To view, visit http://gerrit.cloudera.org:8080/7867
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3f4b517e9816294d70c57110182132adc6cd46d9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [iwyu] fix on

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: [iwyu] fix on <ext/alloc_traits.h>
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7867/2/build-support/iwyu/iwyu-filter.awk
File build-support/iwyu/iwyu-filter.awk:

Line 32: # Usage:
> Any particular reason to explain all this now that there's iwyu.sh? We coul
The sequence described in this file is for the full IWYU run.  Yes, it might be achieved via iwyu.sh as well, but iwyu.sh cannot invoke the tool in parallel, making it very slow (about 3 times slower than regular 'make -j1' run).

I'm thinking to preserve this way of running full/non-incremental IWYU coverage.


http://gerrit.cloudera.org:8080/#/c/7867/2/build-support/iwyu/mappings/libstdcpp.imp
File build-support/iwyu/mappings/libstdcpp.imp:

> Shouldn't this new file be referenced in iwyu.sh?
yes -- it seems I missed that (i.e. I missed that change in this changelist), my bad -- will fix it.


PS2, Line 18:     { include: ["<ext/alloc_traits.h>", private, "<memory>", public ] },
            :     { include: ["<ext/alloc_traits.h>", private, "<condition_variable>", public ] },
            :     { include: ["<ext/alloc_traits.h>", private, "<unordered_map>", public ] },
            :     { include: ["<ext/alloc_traits.h>", private, "<unordered_set>", public ] }
> Why all four recommendations? Why not just one, like <memory>?
It might happen that the file already includes at least one of the rest: <condition_variable>, <unordered_map>, <unordered_set>.  If so, then the <ext/alloc_traits.h> would be included transitively, and there is no need to include <memory>.

As I understand, the idea is that IWYU recommends to include the very first of the listed alternatives (i.e. <memory>) if nothing else is included yet.  If other alternative mapping is  already applied, then IWYU would happily accept that.


-- 
To view, visit http://gerrit.cloudera.org:8080/7867
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f4b517e9816294d70c57110182132adc6cd46d9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [iwyu] fix on

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: [iwyu] fix on <ext/alloc_traits.h>
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7867/2/build-support/iwyu/mappings/libstdcpp.imp
File build-support/iwyu/mappings/libstdcpp.imp:

PS2, Line 18:     { include: ["<ext/alloc_traits.h>", private, "<memory>", public ] },
            :     { include: ["<ext/alloc_traits.h>", private, "<condition_variable>", public ] },
            :     { include: ["<ext/alloc_traits.h>", private, "<unordered_map>", public ] },
            :     { include: ["<ext/alloc_traits.h>", private, "<unordered_set>", public ] }
> Makes sense. In gcc 5.4 <string> would also work (via basic_string.h). Or <
Thank you for digging out those.  I also looked at those for my gcc 5.4.0 at my osx laptop and added <future> header as well.  Now we should assume the list is more accurate than the original one.


-- 
To view, visit http://gerrit.cloudera.org:8080/7867
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f4b517e9816294d70c57110182132adc6cd46d9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] [iwyu] fix on

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: [iwyu] fix on <ext/alloc_traits.h>
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7867/3/src/kudu/tablet/delta_tracker.cc
File src/kudu/tablet/delta_tracker.cc:

Line 53: #include "kudu/util/status.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done


http://gerrit.cloudera.org:8080/#/c/7867/3/src/kudu/util/env-test.cc
File src/kudu/util/env-test.cc:

Line 57: #include "kudu/util/monotime.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7867
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f4b517e9816294d70c57110182132adc6cd46d9
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] [iwyu] fix on

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged.

Change subject: [iwyu] fix on <ext/alloc_traits.h>
......................................................................


[iwyu] fix on <ext/alloc_traits.h>

Fixed the issue where IWYU suggested to include the internal GCC header
<ext/alloc_traits.h>: added additional mapping file which declares the
header as a private one, suggesting to include one of the public header
files of the standard C++ library files.

Change-Id: I3f4b517e9816294d70c57110182132adc6cd46d9
Reviewed-on: http://gerrit.cloudera.org:8080/7867
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M build-support/iwyu/iwyu-filter.awk
M build-support/iwyu/iwyu.sh
A build-support/iwyu/mappings/libstdcpp.imp
M src/kudu/cfile/index_btree.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/common/schema.cc
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/log_cache.cc
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_util.cc
M src/kudu/experiments/merge-test.cc
M src/kudu/gutil/strings/split.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/integration-tests/fuzz-itest.cc
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/master-test.cc
M src/kudu/security/token-test.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/lock_manager-test.cc
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset_tree-test.cc
M src/kudu/tablet/rowset_tree.cc
M src/kudu/tablet/tablet-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tserver/scanners-test.cc
M src/kudu/util/env-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/make_shared.h
M src/kudu/util/mt-threadlocal-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/threadpool-test.cc
40 files changed, 44 insertions(+), 43 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/7867
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I3f4b517e9816294d70c57110182132adc6cd46d9
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] [iwyu] fix on

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: [iwyu] fix on <ext/alloc_traits.h>
......................................................................


Patch Set 5: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/7867
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f4b517e9816294d70c57110182132adc6cd46d9
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] [iwyu] fix on

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: [iwyu] fix on <ext/alloc_traits.h>
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7867/2/build-support/iwyu/iwyu-filter.awk
File build-support/iwyu/iwyu-filter.awk:

Line 32: # Usage:
Any particular reason to explain all this now that there's iwyu.sh? We could just ask people to run that, no?


http://gerrit.cloudera.org:8080/#/c/7867/2/build-support/iwyu/mappings/libstdcpp.imp
File build-support/iwyu/mappings/libstdcpp.imp:

Shouldn't this new file be referenced in iwyu.sh?


PS2, Line 18:     { include: ["<ext/alloc_traits.h>", private, "<memory>", public ] },
            :     { include: ["<ext/alloc_traits.h>", private, "<condition_variable>", public ] },
            :     { include: ["<ext/alloc_traits.h>", private, "<unordered_map>", public ] },
            :     { include: ["<ext/alloc_traits.h>", private, "<unordered_set>", public ] }
Why all four recommendations? Why not just one, like <memory>?


-- 
To view, visit http://gerrit.cloudera.org:8080/7867
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f4b517e9816294d70c57110182132adc6cd46d9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [iwyu] fix on

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: [iwyu] fix on <ext/alloc_traits.h>
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7867/2/build-support/iwyu/iwyu-filter.awk
File build-support/iwyu/iwyu-filter.awk:

Line 32: # Usage:
> The sequence described in this file is for the full IWYU run.  Yes, it migh
Okay.


http://gerrit.cloudera.org:8080/#/c/7867/2/build-support/iwyu/mappings/libstdcpp.imp
File build-support/iwyu/mappings/libstdcpp.imp:

PS2, Line 18:     { include: ["<ext/alloc_traits.h>", private, "<memory>", public ] },
            :     { include: ["<ext/alloc_traits.h>", private, "<condition_variable>", public ] },
            :     { include: ["<ext/alloc_traits.h>", private, "<unordered_map>", public ] },
            :     { include: ["<ext/alloc_traits.h>", private, "<unordered_set>", public ] }
> It might happen that the file already includes at least one of the rest: <c
Makes sense. In gcc 5.4 <string> would also work (via basic_string.h). Or <map>/<set> (via stl_tree.h). Or <deque>/<vector> (via stl_construct.h):

  adar@adar-ThinkPad-T540p:/usr/include/c++/5$ grep -RI ext/alloc_traits.h
  bits/basic_string.h:#include <ext/alloc_traits.h>
  bits/forward_list.h:#include <ext/alloc_traits.h>
  bits/stl_tree.h:#include <ext/alloc_traits.h>
  bits/stl_construct.h:#include <ext/alloc_traits.h>
  ext/debug_allocator.h:#include <ext/alloc_traits.h>
  unordered_set:#include <ext/alloc_traits.h>
  unordered_map:#include <ext/alloc_traits.h>
  debug/safe_container.h:#include <ext/alloc_traits.h>

  adar@adar-ThinkPad-T540p:/usr/include/c++/5$ grep -RI basic_string.h
  string:#include <bits/basic_string.h>

  adar@adar-ThinkPad-T540p:/usr/include/c++/5$ grep -RI stl_tree.h
  ext/rb_tree:#include <bits/stl_tree.h>
  map:#include <bits/stl_tree.h>
  set:#include <bits/stl_tree.h>

  adar@adar-ThinkPad-T540p:/usr/include/c++/5$ grep -RI stl_construct.h
  bits/stl_tempbuf.h:#include <bits/stl_construct.h>
  ext/slist:#include <bits/stl_construct.h>
  ext/rope:#include <bits/stl_construct.h>
  deque:#include <bits/stl_construct.h>
  memory:#include <bits/stl_construct.h>
  vector:#include <bits/stl_construct.h>


-- 
To view, visit http://gerrit.cloudera.org:8080/7867
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f4b517e9816294d70c57110182132adc6cd46d9
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] [iwyu] fix on

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/7867

to look at the new patch set (#5).

Change subject: [iwyu] fix on <ext/alloc_traits.h>
......................................................................

[iwyu] fix on <ext/alloc_traits.h>

Fixed the issue where IWYU suggested to include the internal GCC header
<ext/alloc_traits.h>: added additional mapping file which declares the
header as a private one, suggesting to include one of the public header
files of the standard C++ library files.

Change-Id: I3f4b517e9816294d70c57110182132adc6cd46d9
---
M build-support/iwyu/iwyu-filter.awk
M build-support/iwyu/iwyu.sh
A build-support/iwyu/mappings/libstdcpp.imp
M src/kudu/cfile/index_btree.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/common/schema.cc
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/log_cache.cc
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_util.cc
M src/kudu/experiments/merge-test.cc
M src/kudu/gutil/strings/split.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/integration-tests/fuzz-itest.cc
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/master-test.cc
M src/kudu/security/token-test.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/lock_manager-test.cc
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset_tree-test.cc
M src/kudu/tablet/rowset_tree.cc
M src/kudu/tablet/tablet-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tserver/scanners-test.cc
M src/kudu/util/env-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/make_shared.h
M src/kudu/util/mt-threadlocal-test.cc
M src/kudu/util/subprocess.cc
M src/kudu/util/threadpool-test.cc
40 files changed, 44 insertions(+), 43 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/7867/5
-- 
To view, visit http://gerrit.cloudera.org:8080/7867
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3f4b517e9816294d70c57110182132adc6cd46d9
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot