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/17 03:10:48 UTC
[kudu-CR] [iwyu] update on the internal and boost mappings
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7696
Change subject: [iwyu] update on the internal and boost mappings
......................................................................
[iwyu] update on the internal and boost mappings
Updated the internal and boost mappings to avoid some bogus suggestions
generated by the include-what-you-use tool. Also, shortened the list
of files where IWUY suggestions are 'muted'.
Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2
---
M build-support/iwyu/iwyu-filter.awk
M build-support/iwyu/mappings/boost-all.imp
M thirdparty/patches/llvm-iwyu-include-picker.patch
3 files changed, 21 insertions(+), 103 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/7696/1
--
To view, visit http://gerrit.cloudera.org:8080/7696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [iwyu] update on the internal and boost mappings
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [iwyu] update on the internal and boost mappings
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7696/1//COMMIT_MSG
Commit Message:
PS1, Line 11: IWUY
IWYU
http://gerrit.cloudera.org:8080/#/c/7696/1/build-support/iwyu/mappings/boost-all.imp
File build-support/iwyu/mappings/boost-all.imp:
Line 960: { include: ["<boost/core/explicit_operator_bool.hpp>", private, "<boost/optional/optional.hpp>", public ] },
Are these also imported from upstream? Or your changes?
If the latter, perhaps we should put them in a separate file so it's easier to apply updates from upstream.
http://gerrit.cloudera.org:8080/#/c/7696/1/thirdparty/patches/llvm-iwyu-include-picker.patch
File thirdparty/patches/llvm-iwyu-include-picker.patch:
You need to bump LLVM's patchlevel for this to be incorporated in existing deployments.
--
To view, visit http://gerrit.cloudera.org:8080/7696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2
Gerrit-PatchSet: 1
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-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [iwyu] update on the internal and boost mappings
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7696
to look at the new patch set (#5).
Change subject: [iwyu] update on the internal and boost mappings
......................................................................
[iwyu] update on the internal and boost mappings
Updated the internal and boost mappings to avoid some bogus suggestions
generated by the include-what-you-use tool. Also, shortened the list
of files where IWYU suggestions are 'muted'.
The IWYU patch is updated to deal with int{16,32,64}_t-related quirks.
For some reason, IWYU suggests to use boost headers instead of std
library headers in some .cc files where both <cstdint> and
boost/function.hpp (or boost/optional/optional.hpp) are present.
Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2
---
M build-support/iwyu/iwyu-filter.awk
A build-support/iwyu/mappings/boost-extra.imp
M thirdparty/download-thirdparty.sh
M thirdparty/patches/llvm-iwyu-include-picker.patch
4 files changed, 44 insertions(+), 211 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/7696/5
--
To view, visit http://gerrit.cloudera.org:8080/7696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2
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: Todd Lipcon <to...@apache.org>
[kudu-CR] [iwyu] update on the internal and boost mappings
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [iwyu] update on the internal and boost mappings
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7696/1/thirdparty/patches/llvm-iwyu-include-picker.patch
File thirdparty/patches/llvm-iwyu-include-picker.patch:
> even if the number of patches is the same?
Oh, I see. That's the only way to refresh stale workspaces. All right, I'll bump the patch level up.
--
To view, visit http://gerrit.cloudera.org:8080/7696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2
Gerrit-PatchSet: 1
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: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [iwyu] update on the internal and boost mappings
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/7696
to look at the new patch set (#3).
Change subject: [iwyu] update on the internal and boost mappings
......................................................................
[iwyu] update on the internal and boost mappings
Updated the internal and boost mappings to avoid some bogus suggestions
generated by the include-what-you-use tool. Also, shortened the list
of files where IWYU suggestions are 'muted'.
Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2
---
M build-support/iwyu/iwyu-filter.awk
A build-support/iwyu/mappings/boost-extra.imp
M thirdparty/download-thirdparty.sh
M thirdparty/patches/llvm-iwyu-include-picker.patch
4 files changed, 44 insertions(+), 169 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/7696/3
--
To view, visit http://gerrit.cloudera.org:8080/7696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2
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: Todd Lipcon <to...@apache.org>
[kudu-CR] [iwyu] update on the internal and boost mappings
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged.
Change subject: [iwyu] update on the internal and boost mappings
......................................................................
[iwyu] update on the internal and boost mappings
Updated the internal and boost mappings to avoid some bogus suggestions
generated by the include-what-you-use tool. Also, shortened the list
of files where IWYU suggestions are 'muted'.
The IWYU patch is updated to deal with int{16,32,64}_t-related quirks.
For some reason, IWYU suggests to use boost headers instead of std
library headers in some .cc files where both <cstdint> and
boost/function.hpp (or boost/optional/optional.hpp) are present.
Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2
Reviewed-on: http://gerrit.cloudera.org:8080/7696
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M build-support/iwyu/iwyu-filter.awk
A build-support/iwyu/mappings/boost-extra.imp
M thirdparty/download-thirdparty.sh
M thirdparty/patches/llvm-iwyu-include-picker.patch
4 files changed, 44 insertions(+), 211 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2
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: Todd Lipcon <to...@apache.org>
[kudu-CR] [iwyu] update on the internal and boost mappings
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [iwyu] update on the internal and boost mappings
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2
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: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [iwyu] update on the internal and boost mappings
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [iwyu] update on the internal and boost mappings
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7696/4/thirdparty/patches/llvm-iwyu-include-picker.patch
File thirdparty/patches/llvm-iwyu-include-picker.patch:
Line 3: @@ -123,6 +123,13 @@ const IncludeMapEntry libc_symbol_map[] = {
Is there any sort of provenance for this patch? Without a comment saying where it's coming from / what it's doing, it's a lot harder to go back and remove it later. Is this diff on track to get upstreamed somewhere?
--
To view, visit http://gerrit.cloudera.org:8080/7696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2
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: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [iwyu] update on the internal and boost mappings
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [iwyu] update on the internal and boost mappings
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7696/4/thirdparty/patches/llvm-iwyu-include-picker.patch
File thirdparty/patches/llvm-iwyu-include-picker.patch:
Line 3: @@ -123,6 +123,13 @@ const IncludeMapEntry libc_symbol_map[] = {
> Is there any sort of provenance for this patch? Without a comment saying w
Those are my fixes on the issues I saw while running the include-what-you-use tool on the Kudu sources. I'm planning to eventually merge them into the IWYU upstream repo.
I'll add a comment into the commit message explaining why I need them here.
--
To view, visit http://gerrit.cloudera.org:8080/7696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2
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: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [iwyu] update on the internal and boost mappings
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [iwyu] update on the internal and boost mappings
......................................................................
Patch Set 4: Code-Review+2
But I agree with Dan's provenance comment.
--
To view, visit http://gerrit.cloudera.org:8080/7696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2
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: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [iwyu] update on the internal and boost mappings
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/7696
to look at the new patch set (#4).
Change subject: [iwyu] update on the internal and boost mappings
......................................................................
[iwyu] update on the internal and boost mappings
Updated the internal and boost mappings to avoid some bogus suggestions
generated by the include-what-you-use tool. Also, shortened the list
of files where IWYU suggestions are 'muted'.
Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2
---
M build-support/iwyu/iwyu-filter.awk
A build-support/iwyu/mappings/boost-extra.imp
M thirdparty/download-thirdparty.sh
M thirdparty/patches/llvm-iwyu-include-picker.patch
4 files changed, 44 insertions(+), 211 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/7696/4
--
To view, visit http://gerrit.cloudera.org:8080/7696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2
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: Todd Lipcon <to...@apache.org>
[kudu-CR] [iwyu] update on the internal and boost mappings
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [iwyu] update on the internal and boost mappings
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7696/1/build-support/iwyu/mappings/boost-all.imp
File build-support/iwyu/mappings/boost-all.imp:
Line 960: { include: ["<boost/core/explicit_operator_bool.hpp>", private, "<boost/optional/optional.hpp>", public ] },
> Are these also imported from upstream? Or your changes?
These are my changes. OK, keeping them separate would be a better choice, agreed. From the other side, I could try to re-generate the whole file.
http://gerrit.cloudera.org:8080/#/c/7696/1/thirdparty/patches/llvm-iwyu-include-picker.patch
File thirdparty/patches/llvm-iwyu-include-picker.patch:
> You need to bump LLVM's patchlevel for this to be incorporated in existing
even if the number of patches is the same?
--
To view, visit http://gerrit.cloudera.org:8080/7696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2
Gerrit-PatchSet: 1
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: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [iwyu] update on the internal and boost mappings
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [iwyu] update on the internal and boost mappings
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7696/1//COMMIT_MSG
Commit Message:
PS1, Line 11: IWUY
> IWYU
Done
http://gerrit.cloudera.org:8080/#/c/7696/1/build-support/iwyu/mappings/boost-all.imp
File build-support/iwyu/mappings/boost-all.imp:
Line 960: { include: ["<boost/core/explicit_operator_bool.hpp>", private, "<boost/optional/optional.hpp>", public ] },
> Are these also imported from upstream? Or your changes?
Done
http://gerrit.cloudera.org:8080/#/c/7696/1/thirdparty/patches/llvm-iwyu-include-picker.patch
File thirdparty/patches/llvm-iwyu-include-picker.patch:
> You need to bump LLVM's patchlevel for this to be incorporated in existing
Done
--
To view, visit http://gerrit.cloudera.org:8080/7696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2
Gerrit-PatchSet: 1
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: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [iwyu] update on the internal and boost mappings
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/7696
to look at the new patch set (#2).
Change subject: [iwyu] update on the internal and boost mappings
......................................................................
[iwyu] update on the internal and boost mappings
Updated the internal and boost mappings to avoid some bogus suggestions
generated by the include-what-you-use tool. Also, shortened the list
of files where IWUY suggestions are 'muted'.
Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2
---
M build-support/iwyu/iwyu-filter.awk
A build-support/iwyu/mappings/boost-extra.imp
M thirdparty/download-thirdparty.sh
M thirdparty/patches/llvm-iwyu-include-picker.patch
4 files changed, 44 insertions(+), 169 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/7696/2
--
To view, visit http://gerrit.cloudera.org:8080/7696
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2
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: Todd Lipcon <to...@apache.org>