Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/15058 )
Change subject: build: restrict clang version, prefer lld, enable thinlto ...................................................................... Patch Set 3: (12 comments) http://gerrit.cloudera.org:8080/#/c/15058/1/CMakeLists.txt File CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/15058/1/CMakeLists.txt@239 PS1, Line 239: > May want to recommend the ccache wrappers in build-support/ccache-clang ins Done http://gerrit.cloudera.org:8080/#/c/15058/1/CMakeLists.txt@416 PS1, Line 416: # Doesn't make much sense to do LTO on a non-release build -- it's slow > Could you explain why in a comment? Done http://gerrit.cloudera.org:8080/#/c/15058/1/CMakeLists.txt@419 PS1, Line 419: endif() > You sure you want PROJECT_BINARY_DIR? We don't call project() so what actua Done http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake File cmake_modules/KuduLinker.cmake: http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@20 PS1, Line 20: # Search candidate linkers in the order of decreasing speed and functionality. : # In particular, lld is the best choice since it's quite fast and supports : # ThinLTO, etc. : foreach(candid > Want to move this down to where it's actually needed? Done http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@25 PS1, Line 25: set(candidate_flag "") > May want to add a comment explaining the rationale for the ordering (i.e. p Done http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@26 PS1, Line 26: else() : set(candidate_flag "-fuse-ld=${candidate_linker}") : endif() : GET_LINKER_VERSION("${candidate_flag}") : if(NOT > Could you get away with this? nope, since ${candidate_flag} will hang around from a previous loop iteration (cmake has function scope, not lexical scope) http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@56 PS1, Line 56: > How was it fixed? added a comment. Just tested this experimentally, didn't locate the actual patch. http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@80 PS1, Line 80: r > remove Done http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@85 PS1, Line 85: # LINKER_FAMILY: lld/ld/gold > Maybe adjust the message a bit if ${ARGN} is empty (i.e. the default linker Done http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@89 PS1, Line 89: if(ARGN) : message(STATUS "Checking linker version with cflags: ${ARGN}") : else() > Nit: move this down to just above L93. Done http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@102 PS1, Line 102: message(SEND_ERROR "Could not extract GNU gold version. " > Would be great if you could provide some sample text to help illustrate how Done http://gerrit.cloudera.org:8080/#/c/15058/1/cmake_modules/KuduLinker.cmake@118 PS1, Line 118: set(LINKER_FAMILY "ld") > Typo here (I think you meant to omit PARENT_SCOPE?) Surprised it didn't giv think it ended up fine bceause any non-empty string is true according to cmake -- To view, visit http://gerrit.cloudera.org:8080/15058 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I09ded0c44c9f7a6839489f0abf5baa4eaf1971f0 Gerrit-Change-Number: 15058 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Fri, 17 Jan 2020 21:46:08 +0000 Gerrit-HasComments: Yes
