You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2020/03/19 08:25:15 UTC
[kudu-CR] WIP: iwyu: standardize on libc++
Hello Alexey Serbin, Andrew Wong, Grant Henke,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/15492
to review the following change.
Change subject: WIP: iwyu: standardize on libc++
......................................................................
WIP: iwyu: standardize on libc++
A common IWYU pain point is that the set of recommendations you get for C++
standard library headers differs depending on the system you run on.
Pre-commit currently uses Ubuntu 14.10, so to guarantee a pass in pre-commit
one must run IWYU on an Ubuntu 14.10 VM.
Instead, let's standardize on libc++ for C++ standard library headers. Even
though Kudu itself is built against the local libstdc++, the standard
library classes we use should be available in the same "public" headers
regardless of implementation. And this way, all runs of IWYU will produce
the same recommendations.
A more complete solution would be to standardize _all_ of Kudu on clang and
libc++. That's a larger piece of work with its own issues (e.g. is it safe
to statically link libc++ into the C++ client library?).
As with anything related to IWYU, some mappings had to be massaged, both for
libc++ itself as well as interesting things I noticed on my local system.
Note: strictly speaking we don't need to _build_ libc++, but it's a quick
build and this is the easiest way to get the appropriate set of headers into
thirdparty/installed/<prefix>/include.
WIP because some of the recommendations are still wonky. For example, IWYU
wants us to include <new> in every file that uses operator new. At first I
mapped this to <memory>, but that's not actually correct. Then I tried to
suppress the recommendation, but that's only possible with
"IWYU pragma: no_include" in each affected file, which is even worse. Seems
like some codebases are comfortable including <new> for such usage; should
we follow suit?
Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
---
M build-support/iwyu.py
M build-support/iwyu/iwyu_tool.py
A build-support/iwyu/mappings/libcxx-extra.imp
A build-support/iwyu/mappings/libcxx.imp
M build-support/iwyu/mappings/system-linux.imp
M src/kudu/util/thread.cc
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
8 files changed, 153 insertions(+), 39 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/15492/1
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
[kudu-CR] iwyu: standardize on libc++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/15492 )
Change subject: iwyu: standardize on libc++
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 8
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] iwyu: standardize on libc++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15492
to look at the new patch set (#4).
Change subject: iwyu: standardize on libc++
......................................................................
iwyu: standardize on libc++
A common IWYU pain point is that the set of recommendations you get for C++
standard library headers differs depending on the system you run on.
Pre-commit currently uses Ubuntu 14.10, so to guarantee a pass in pre-commit
one must run IWYU on an Ubuntu 14.10 VM.
Instead, let's standardize on libc++ for C++ standard library headers. Even
though Kudu itself is built against the system's libstdc++, the standard
library classes we use should be available in the same "public" headers
regardless of implementation. And this way, all runs of IWYU will produce
the same recommendations. A more comprehensive solution would be to
standardize _all_ of Kudu on clang and libc++. That's a larger piece of work
with its own issues (e.g. is it safe to statically link libc++ into the C++
client library?).
As with anything related to IWYU, some mappings had to be massaged, both for
libc++ itself as well as interesting things I noticed on my local system. I
also included a patch to fix a bug in IWYU dealing with false <new>
recommendations when compiling a codebase with -fsized-deallocation.
Finally, I removed the "muted" file stuff from iwyu.py: let's work to make
all of these files IWYU-safe instead.
Note: strictly speaking we don't need to _build_ libc++, but it's a quick
build and this is the easiest way to get the appropriate set of headers into
thirdparty/installed/<prefix>/include.
Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
---
M build-support/iwyu.py
M build-support/iwyu/iwyu_tool.py
A build-support/iwyu/mappings/libcxx-extra.imp
A build-support/iwyu/mappings/libcxx.imp
M build-support/iwyu/mappings/system-linux.imp
M src/kudu/util/thread.cc
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
A thirdparty/patches/llvm-iwyu-sized-deallocation.patch
10 files changed, 288 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/15492/4
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] iwyu: standardize on libc++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15492 )
Change subject: iwyu: standardize on libc++
......................................................................
Patch Set 10:
> Looks good to me and I verified it works fine on CentOS8.
>
> However, there are issues when building the code on macOS with the
> clang from the third-party. I think that's might be something with
> my environment and not exactly related to this patch. I'll try to
> clarify on that.
Meanwhile, I think this patch is good to go. Thank you for implementing this!
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 10
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 26 Mar 2020 00:03:17 +0000
Gerrit-HasComments: No
[kudu-CR] iwyu: standardize on libc++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15492 )
Change subject: iwyu: standardize on libc++
......................................................................
Patch Set 10: Code-Review+2
Looks good to me and I verified it works fine on CentOS8.
However, there are issues when building the code on macOS with the clang from the third-party. I think that's might be something with my environment and not exactly related to this patch. I'll try to clarify on that.
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 10
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 26 Mar 2020 00:02:52 +0000
Gerrit-HasComments: No
[kudu-CR] WIP: iwyu: standardize on libc++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15492 )
Change subject: WIP: iwyu: standardize on libc++
......................................................................
Patch Set 2:
(2 comments)
Thank you very much for doing this!
http://gerrit.cloudera.org:8080/#/c/15492/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15492/2//COMMIT_MSG@35
PS2, Line 35: Seems
: like some codebases are comfortable including <new> for such usage; should
: we follow suit?
> Patching IWYU to omit <new> would be better.
I'm OK with either approach here. Probably, not including the <new> header could save a bit of compilation time, but I don't think that would be much.
As per https://en.cppreference.com/w/cpp/memory/new/operator_new , signatures like 1-4 should not require inclusion of the <new> header:
The versions (1-4) are implicitly declared in each translation unit even if the <new> header is not included.
Patching iwyu to avoid the recommendation of including the <new> header seems safe, even if using some coarse blanket-style rule that ignores other signatures (5-22). There should be a compiler error if not including required <new> as required anyways.
http://gerrit.cloudera.org:8080/#/c/15492/2/build-support/iwyu/mappings/system-linux.imp
File build-support/iwyu/mappings/system-linux.imp:
http://gerrit.cloudera.org:8080/#/c/15492/2/build-support/iwyu/mappings/system-linux.imp@41
PS2, Line 41: <time.h>
> <ctime> instead?
It might be tricky with IWYU if doing so, but it might work, indeed. Last time I tried that with 'clockid_t' (se line 37), it didn't work.
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 19 Mar 2020 18:38:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: iwyu: standardize on libc++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15492
to look at the new patch set (#3).
Change subject: WIP: iwyu: standardize on libc++
......................................................................
WIP: iwyu: standardize on libc++
A common IWYU pain point is that the set of recommendations you get for C++
standard library headers differs depending on the system you run on.
Pre-commit currently uses Ubuntu 14.10, so to guarantee a pass in pre-commit
one must run IWYU on an Ubuntu 14.10 VM.
Instead, let's standardize on libc++ for C++ standard library headers. Even
though Kudu itself is built against the local libstdc++, the standard
library classes we use should be available in the same "public" headers
regardless of implementation. And this way, all runs of IWYU will produce
the same recommendations.
A more complete solution would be to standardize _all_ of Kudu on clang and
libc++. That's a larger piece of work with its own issues (e.g. is it safe
to statically link libc++ into the C++ client library?).
As with anything related to IWYU, some mappings had to be massaged, both for
libc++ itself as well as interesting things I noticed on my local system.
Note: strictly speaking we don't need to _build_ libc++, but it's a quick
build and this is the easiest way to get the appropriate set of headers into
thirdparty/installed/<prefix>/include.
WIP because some of the recommendations are still wonky. For example, IWYU
wants us to include <new> in every file that uses operator new. At first I
mapped this to <memory>, but that's not actually correct. Then I tried to
suppress the recommendation, but that's only possible with
"IWYU pragma: no_include" in each affected file, which is even worse. Seems
like some codebases are comfortable including <new> for such usage; should
we follow suit?
Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
---
M build-support/iwyu.py
M build-support/iwyu/iwyu_tool.py
A build-support/iwyu/mappings/libcxx-extra.imp
A build-support/iwyu/mappings/libcxx.imp
M build-support/iwyu/mappings/system-linux.imp
M src/kudu/util/thread.cc
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
8 files changed, 169 insertions(+), 48 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/15492/3
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] iwyu: standardize on libc++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15492
to look at the new patch set (#7).
Change subject: iwyu: standardize on libc++
......................................................................
iwyu: standardize on libc++
A common IWYU pain point is that the set of recommendations you get for C++
standard library headers differs depending on the system you run on.
Pre-commit currently uses Ubuntu 14.10, so to guarantee a pass in pre-commit
one must run IWYU on an Ubuntu 14.10 VM.
Instead, let's standardize on libc++ for C++ standard library headers. Even
though Kudu itself is built against the system's libstdc++, the standard
library classes we use should be available in the same "public" headers
regardless of implementation. And this way, all runs of IWYU will produce
the same recommendations. A more comprehensive solution would be to
standardize _all_ of Kudu on clang and libc++. That's a larger piece of work
with its own issues (e.g. is it safe to statically link libc++ into the C++
client library?).
This patch also introduces mappings for libc++. Some are from the IWYU repo
while others are new for Kudu. I also included a patch to fix a bug in IWYU
dealing with false <new> recommendations when processing a codebase using
-fsized-deallocation. Finally, I removed almost all of the "muted" files
from iwyu.py; the few remaining instances trip up IWYU without recourse.
Note: strictly speaking we don't need to _build_ libc++, but it's a quick
build and this is the easiest way to get the appropriate set of headers into
thirdparty/installed/<prefix>/include.
Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
---
M build-support/iwyu.py
M build-support/iwyu/iwyu_tool.py
A build-support/iwyu/mappings/libcxx-extra.imp
A build-support/iwyu/mappings/libcxx.imp
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
A thirdparty/patches/llvm-iwyu-sized-deallocation.patch
8 files changed, 283 insertions(+), 45 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/15492/7
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] iwyu: standardize on libc++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15492 )
Change subject: iwyu: standardize on libc++
......................................................................
iwyu: standardize on libc++
A common IWYU pain point is that the set of recommendations you get for C++
standard library headers differs depending on the system you run on.
Pre-commit currently uses Ubuntu 14.10, so to guarantee a pass in pre-commit
one must run IWYU on an Ubuntu 14.10 VM.
Instead, let's standardize on libc++ for C++ standard library headers. Even
though Kudu itself is built against the system's libstdc++, the standard
library classes we use should be available in the same "public" headers
regardless of implementation. And this way, all runs of IWYU will produce
the same recommendations. A more comprehensive solution would be to
standardize _all_ of Kudu on clang and libc++. That's a larger piece of work
with its own issues (e.g. is it safe to statically link libc++ into the C++
client library?).
This patch also introduces mappings for libc++. Some are from the IWYU repo
while others are new for Kudu. I also included a patch to fix a bug in IWYU
dealing with false <new> recommendations when processing a codebase using
-fsized-deallocation[1]. Finally, I removed almost all of the "muted" files
from iwyu.py; the few remaining instances trip up IWYU without recourse.
Note: strictly speaking we don't need to _build_ libc++, but it's a quick
build and this is the easiest way to get the appropriate set of headers into
thirdparty/installed/<prefix>/include.
1. https://github.com/include-what-you-use/include-what-you-use/issues/777
Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Reviewed-on: http://gerrit.cloudera.org:8080/15492
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
---
M build-support/iwyu.py
M build-support/iwyu/iwyu_tool.py
A build-support/iwyu/mappings/libcxx-extra.imp
A build-support/iwyu/mappings/libcxx.imp
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
A thirdparty/patches/llvm-iwyu-sized-deallocation.patch
8 files changed, 283 insertions(+), 45 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 12
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] iwyu: standardize on libc++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15492 )
Change subject: iwyu: standardize on libc++
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 11
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 27 Mar 2020 05:18:18 +0000
Gerrit-HasComments: No
[kudu-CR] iwyu: standardize on libc++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Andrew Wong, Grant Henke, Bankim Bhavsar, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15492
to look at the new patch set (#9).
Change subject: iwyu: standardize on libc++
......................................................................
iwyu: standardize on libc++
A common IWYU pain point is that the set of recommendations you get for C++
standard library headers differs depending on the system you run on.
Pre-commit currently uses Ubuntu 14.10, so to guarantee a pass in pre-commit
one must run IWYU on an Ubuntu 14.10 VM.
Instead, let's standardize on libc++ for C++ standard library headers. Even
though Kudu itself is built against the system's libstdc++, the standard
library classes we use should be available in the same "public" headers
regardless of implementation. And this way, all runs of IWYU will produce
the same recommendations. A more comprehensive solution would be to
standardize _all_ of Kudu on clang and libc++. That's a larger piece of work
with its own issues (e.g. is it safe to statically link libc++ into the C++
client library?).
This patch also introduces mappings for libc++. Some are from the IWYU repo
while others are new for Kudu. I also included a patch to fix a bug in IWYU
dealing with false <new> recommendations when processing a codebase using
-fsized-deallocation[1]. Finally, I removed almost all of the "muted" files
from iwyu.py; the few remaining instances trip up IWYU without recourse.
Note: strictly speaking we don't need to _build_ libc++, but it's a quick
build and this is the easiest way to get the appropriate set of headers into
thirdparty/installed/<prefix>/include.
1. https://github.com/include-what-you-use/include-what-you-use/issues/777
Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
---
M build-support/iwyu.py
M build-support/iwyu/iwyu_tool.py
A build-support/iwyu/mappings/libcxx-extra.imp
A build-support/iwyu/mappings/libcxx.imp
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
A thirdparty/patches/llvm-iwyu-sized-deallocation.patch
8 files changed, 283 insertions(+), 45 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/15492/9
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 9
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] iwyu: standardize on libc++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15492 )
Change subject: iwyu: standardize on libc++
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15492/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15492/5//COMMIT_MSG@12
PS5, Line 12: Ubuntu 14.10 VM
> The recommendations you got on CentOS 6.6 don't always match that of Ubuntu
I see, probably I forgot about those.
http://gerrit.cloudera.org:8080/#/c/15492/5/src/kudu/util/thread.cc
File src/kudu/util/thread.cc:
http://gerrit.cloudera.org:8080/#/c/15492/5/src/kudu/util/thread.cc@122
PS5, Line 122: rusage
> Unrelated to IWYU; just consistency with the rest of the codebase. I ended
Indeed, I see those are not in PS7.
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 24 Mar 2020 19:00:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: iwyu: standardize on libc++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15492 )
Change subject: WIP: iwyu: standardize on libc++
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15492/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15492/2//COMMIT_MSG@35
PS2, Line 35: Seems
: like some codebases are comfortable including <new> for such usage; should
: we follow suit?
Another option is to patch IWYU to forcefully omit this inclusion.
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 19 Mar 2020 08:33:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] iwyu: standardize on libc++
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/15492 )
Change subject: iwyu: standardize on libc++
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15492/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15492/7//COMMIT_MSG@26
PS7, Line 26: -fsized-deallocation. Finally, I removed almost all of the "muted" files
did you submit this patch upstream somewhere?
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 24 Mar 2020 18:01:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] iwyu: standardize on libc++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/15492 )
Change subject: iwyu: standardize on libc++
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
[kudu-CR] WIP: iwyu: standardize on libc++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15492 )
Change subject: WIP: iwyu: standardize on libc++
......................................................................
Patch Set 1:
You can see the output of iwyu.py --all here: https://pastebin.com/FXM7Pa9Q
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 19 Mar 2020 08:28:02 +0000
Gerrit-HasComments: No
[kudu-CR] iwyu: standardize on libc++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15492 )
Change subject: iwyu: standardize on libc++
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15492/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15492/5//COMMIT_MSG@12
PS5, Line 12: Ubuntu 14.10 VM
> Not sure whether it's worth noting it here, but it also works for CentOS 6.
The recommendations you got on CentOS 6.6 don't always match that of Ubuntu 14.10. In particular, I've seen CentOS 6.6 tell me to add/remove either <utility> or <algorithm>, and then Ubuntu 14.10 asks me to undo it.
http://gerrit.cloudera.org:8080/#/c/15492/5/src/kudu/util/thread.cc
File src/kudu/util/thread.cc:
http://gerrit.cloudera.org:8080/#/c/15492/5/src/kudu/util/thread.cc@122
PS5, Line 122: rusage
> I'm curious, what is the significance of this change? Was it necessary to
Unrelated to IWYU; just consistency with the rest of the codebase. I ended up moving it into the next patch anyway.
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 24 Mar 2020 17:18:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] iwyu: standardize on libc++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15492 )
Change subject: iwyu: standardize on libc++
......................................................................
Patch Set 4:
(3 comments)
New IWYU run on all files: https://pastebin.com/Mcyd3pfF
http://gerrit.cloudera.org:8080/#/c/15492/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15492/2//COMMIT_MSG@35
PS2, Line 35:
:
:
> I'm OK with either approach here. Probably, not including the <new> heade
I found the underlying bug in IWYU and fixed it. This patch includes that fix as a new TP patch for IWYU.
http://gerrit.cloudera.org:8080/#/c/15492/2/build-support/iwyu.py
File build-support/iwyu.py:
http://gerrit.cloudera.org:8080/#/c/15492/2/build-support/iwyu.py@58
PS2, Line 58: _FIX_INCLUDES_STYLE_FL
> Removed files no longer need to be muted?
I wasn't sure yet if this should be a permanent change; I just wanted IWYU to run on all files.
But now I agree. Done.
http://gerrit.cloudera.org:8080/#/c/15492/2/build-support/iwyu/mappings/system-linux.imp
File build-support/iwyu/mappings/system-linux.imp:
http://gerrit.cloudera.org:8080/#/c/15492/2/build-support/iwyu/mappings/system-linux.imp@41
PS2, Line 41: <time.h>
> It might be tricky with IWYU if doing so, but it might work, indeed. Last
I don't think <ctime> makes sense here; we're talking about a pure C struct whose definition we want to draw from the appropriate C header.
IWYU has a built-in mapping that looks like this:
{ include: ["<time.h>", public, "<ctime>", public] },
That'll make <ctime> a usable substitute for <time.h> (i.e. IWYU won't ask you to change it to <time.h>). But since <time.h> is defined as public, we can't define it as private and force IWYU to always substitute.
That said, clang-tidy's modernize-deprecated-headers check can handle this for us, we just haven't enabled it.
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 23 Mar 2020 22:12:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] iwyu: standardize on libc++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Andrew Wong, Grant Henke, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15492
to look at the new patch set (#6).
Change subject: iwyu: standardize on libc++
......................................................................
iwyu: standardize on libc++
A common IWYU pain point is that the set of recommendations you get for C++
standard library headers differs depending on the system you run on.
Pre-commit currently uses Ubuntu 14.10, so to guarantee a pass in pre-commit
one must run IWYU on an Ubuntu 14.10 VM.
Instead, let's standardize on libc++ for C++ standard library headers. Even
though Kudu itself is built against the system's libstdc++, the standard
library classes we use should be available in the same "public" headers
regardless of implementation. And this way, all runs of IWYU will produce
the same recommendations. A more comprehensive solution would be to
standardize _all_ of Kudu on clang and libc++. That's a larger piece of work
with its own issues (e.g. is it safe to statically link libc++ into the C++
client library?).
This patch also introduces mappings for libc++. Some are from the IWYU repo
while others are new for Kudu. I also included a patch to fix a bug in IWYU
dealing with false <new> recommendations when processing a codebase using
-fsized-deallocation. Finally, I removed almost all of the "muted" files
from iwyu.py; the few remaining instances trip up IWYU without recourse.
Note: strictly speaking we don't need to _build_ libc++, but it's a quick
build and this is the easiest way to get the appropriate set of headers into
thirdparty/installed/<prefix>/include.
Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
---
M build-support/iwyu.py
M build-support/iwyu/iwyu_tool.py
A build-support/iwyu/mappings/libcxx-extra.imp
A build-support/iwyu/mappings/libcxx.imp
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
A thirdparty/patches/llvm-iwyu-sized-deallocation.patch
8 files changed, 284 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/15492/6
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
[kudu-CR] WIP: iwyu: standardize on libc++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15492
to look at the new patch set (#2).
Change subject: WIP: iwyu: standardize on libc++
......................................................................
WIP: iwyu: standardize on libc++
A common IWYU pain point is that the set of recommendations you get for C++
standard library headers differs depending on the system you run on.
Pre-commit currently uses Ubuntu 14.10, so to guarantee a pass in pre-commit
one must run IWYU on an Ubuntu 14.10 VM.
Instead, let's standardize on libc++ for C++ standard library headers. Even
though Kudu itself is built against the local libstdc++, the standard
library classes we use should be available in the same "public" headers
regardless of implementation. And this way, all runs of IWYU will produce
the same recommendations.
A more complete solution would be to standardize _all_ of Kudu on clang and
libc++. That's a larger piece of work with its own issues (e.g. is it safe
to statically link libc++ into the C++ client library?).
As with anything related to IWYU, some mappings had to be massaged, both for
libc++ itself as well as interesting things I noticed on my local system.
Note: strictly speaking we don't need to _build_ libc++, but it's a quick
build and this is the easiest way to get the appropriate set of headers into
thirdparty/installed/<prefix>/include.
WIP because some of the recommendations are still wonky. For example, IWYU
wants us to include <new> in every file that uses operator new. At first I
mapped this to <memory>, but that's not actually correct. Then I tried to
suppress the recommendation, but that's only possible with
"IWYU pragma: no_include" in each affected file, which is even worse. Seems
like some codebases are comfortable including <new> for such usage; should
we follow suit?
Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
---
M build-support/iwyu.py
M build-support/iwyu/iwyu_tool.py
A build-support/iwyu/mappings/libcxx-extra.imp
A build-support/iwyu/mappings/libcxx.imp
M build-support/iwyu/mappings/system-linux.imp
M src/kudu/util/thread.cc
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
8 files changed, 153 insertions(+), 39 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/15492/2
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] iwyu: standardize on libc++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15492 )
Change subject: iwyu: standardize on libc++
......................................................................
Patch Set 8: Verified+1
Overriding Jenkins, flaky Java test.
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 8
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 25 Mar 2020 04:31:14 +0000
Gerrit-HasComments: No
[kudu-CR] iwyu: standardize on libc++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15492 )
Change subject: iwyu: standardize on libc++
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15492/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15492/7//COMMIT_MSG@26
PS7, Line 26: -fsized-deallocation. Finally, I removed almost all of the "muted" files
> did you submit this patch upstream somewhere?
Yeah: https://github.com/include-what-you-use/include-what-you-use/pull/778.
I guess I should link to it in this commit.
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 24 Mar 2020 18:02:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: iwyu: standardize on libc++
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15492 )
Change subject: WIP: iwyu: standardize on libc++
......................................................................
Patch Set 2:
(3 comments)
Thanks for addressing this issue, Adar.
http://gerrit.cloudera.org:8080/#/c/15492/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15492/2//COMMIT_MSG@35
PS2, Line 35: Seems
: like some codebases are comfortable including <new> for such usage; should
: we follow suit?
> Another option is to patch IWYU to forcefully omit this inclusion.
Patching IWYU to omit <new> would be better.
http://gerrit.cloudera.org:8080/#/c/15492/2/build-support/iwyu.py
File build-support/iwyu.py:
http://gerrit.cloudera.org:8080/#/c/15492/2/build-support/iwyu.py@58
PS2, Line 58: _MUTED_FILES = set([])
Removed files no longer need to be muted?
http://gerrit.cloudera.org:8080/#/c/15492/2/build-support/iwyu/mappings/system-linux.imp
File build-support/iwyu/mappings/system-linux.imp:
http://gerrit.cloudera.org:8080/#/c/15492/2/build-support/iwyu/mappings/system-linux.imp@41
PS2, Line 41: <time.h>
<ctime> instead?
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 19 Mar 2020 17:15:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] iwyu: standardize on libc++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15492 )
Change subject: iwyu: standardize on libc++
......................................................................
Patch Set 5: Verified+1
Overriding Jenkins, filed KUDU-3092 for the flaky test.
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 24 Mar 2020 06:54:50 +0000
Gerrit-HasComments: No
[kudu-CR] iwyu: standardize on libc++
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15492 )
Change subject: iwyu: standardize on libc++
......................................................................
Patch Set 7: Code-Review+2
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15492/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15492/2//COMMIT_MSG@35
PS2, Line 35:
:
:
> I found the underlying bug in IWYU and fixed it. This patch includes that f
That's great, thank you for addressing this.
http://gerrit.cloudera.org:8080/#/c/15492/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15492/5//COMMIT_MSG@12
PS5, Line 12: Ubuntu 14.10 VM
Not sure whether it's worth noting it here, but it also works for CentOS 6.6 now and I don't recall a time when the result was different between Ubuntu 14.10 and CentOS 6.6 when running IWYU.
But it's true that the list of the systems to run IWYU validation is really limited.
http://gerrit.cloudera.org:8080/#/c/15492/5/src/kudu/util/thread.cc
File src/kudu/util/thread.cc:
http://gerrit.cloudera.org:8080/#/c/15492/5/src/kudu/util/thread.cc@122
PS5, Line 122: rusage
I'm curious, what is the significance of this change? Was it necessary to make IWYU happy?
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 24 Mar 2020 15:01:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] iwyu: standardize on libc++
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Grant Henke, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15492
to look at the new patch set (#5).
Change subject: iwyu: standardize on libc++
......................................................................
iwyu: standardize on libc++
A common IWYU pain point is that the set of recommendations you get for C++
standard library headers differs depending on the system you run on.
Pre-commit currently uses Ubuntu 14.10, so to guarantee a pass in pre-commit
one must run IWYU on an Ubuntu 14.10 VM.
Instead, let's standardize on libc++ for C++ standard library headers. Even
though Kudu itself is built against the system's libstdc++, the standard
library classes we use should be available in the same "public" headers
regardless of implementation. And this way, all runs of IWYU will produce
the same recommendations. A more comprehensive solution would be to
standardize _all_ of Kudu on clang and libc++. That's a larger piece of work
with its own issues (e.g. is it safe to statically link libc++ into the C++
client library?).
As with anything related to IWYU, some mappings had to be massaged, both for
libc++ itself as well as interesting things I noticed on my local system. I
also included a patch to fix a bug in IWYU dealing with false <new>
recommendations when compiling a codebase with -fsized-deallocation.
Finally, I removed the "muted" file stuff from iwyu.py: let's work to make
all of these files IWYU-safe instead.
Note: strictly speaking we don't need to _build_ libc++, but it's a quick
build and this is the easiest way to get the appropriate set of headers into
thirdparty/installed/<prefix>/include.
Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
---
M build-support/iwyu.py
M build-support/iwyu/iwyu_tool.py
A build-support/iwyu/mappings/libcxx-extra.imp
A build-support/iwyu/mappings/libcxx.imp
M build-support/iwyu/mappings/system-linux.imp
M src/kudu/util/thread.cc
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
A thirdparty/patches/llvm-iwyu-sized-deallocation.patch
10 files changed, 295 insertions(+), 55 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/15492/5
--
To view, visit http://gerrit.cloudera.org:8080/15492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic807745271642b3d5d80ea0ad9bc413bdb0e34b5
Gerrit-Change-Number: 15492
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)