[kudu-CR] iwyu: standardize on libc++
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 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//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 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] iwyu: standardize on libc++
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 27 Mar 2020 05:18:18 + Gerrit-HasComments: No
[kudu-CR] iwyu: standardize on libc++
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 26 Mar 2020 00:03:17 + Gerrit-HasComments: No
[kudu-CR] iwyu: standardize on libc++
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 26 Mar 2020 00:02:52 + Gerrit-HasComments: No
[kudu-CR] iwyu: standardize on libc++
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 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//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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Todd Lipcon
[kudu-CR] iwyu: standardize on libc++
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Todd Lipcon
[kudu-CR] iwyu: standardize on libc++
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 25 Mar 2020 04:31:14 + Gerrit-HasComments: No
[kudu-CR] iwyu: standardize on libc++
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 24 Mar 2020 19:00:01 + Gerrit-HasComments: Yes
[kudu-CR] iwyu: standardize on libc++
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 24 Mar 2020 18:02:30 + Gerrit-HasComments: Yes
[kudu-CR] iwyu: standardize on libc++
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 24 Mar 2020 18:01:05 + Gerrit-HasComments: Yes
[kudu-CR] iwyu: standardize on libc++
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 or , 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 24 Mar 2020 17:18:39 + Gerrit-HasComments: Yes
[kudu-CR] iwyu: standardize on libc++
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 24 Mar 2020 15:01:42 + Gerrit-HasComments: Yes
[kudu-CR] iwyu: standardize on libc++
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 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//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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] iwyu: standardize on libc++
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 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//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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke
[kudu-CR] iwyu: standardize on libc++
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 24 Mar 2020 06:54:50 + Gerrit-HasComments: No
[kudu-CR] iwyu: standardize on libc++
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke
[kudu-CR] iwyu: standardize on libc++
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 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//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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] iwyu: standardize on libc++
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 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:
> It might be tricky with IWYU if doing so, but it might work, indeed. Last
I don't think 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: ["", public, "", public] },
That'll make a usable substitute for (i.e. IWYU won't ask you
to change it to ). But since 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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 23 Mar 2020 22:12:05 +
Gerrit-HasComments: Yes
[kudu-CR] iwyu: standardize on libc++
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 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//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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
