You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/09/20 06:43:22 UTC
[kudu-CR] Update kudu-lint to latest LLVM APIs
Todd Lipcon has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4475
Change subject: Update kudu-lint to latest LLVM APIs
......................................................................
Update kudu-lint to latest LLVM APIs
Also removed the 'unused Status' matcher, since this is doable using the
built-in WARN_UNUSED_RESULT attribute.
Change-Id: I7c69be84cad56df71cc1bb2a3a89ada5d2167665
---
M build-support/tools/kudu-lint/CMakeLists.txt
M build-support/tools/kudu-lint/README
M build-support/tools/kudu-lint/kudu-lint.cc
3 files changed, 16 insertions(+), 70 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/4475/1
--
To view, visit http://gerrit.cloudera.org:8080/4475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7c69be84cad56df71cc1bb2a3a89ada5d2167665
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
[kudu-CR] Update kudu-lint to LLVM 3.9
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Update kudu-lint to LLVM 3.9
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4475/2/build-support/tools/kudu-lint/CMakeLists.txt
File build-support/tools/kudu-lint/CMakeLists.txt:
Line 23: find_package(Clang REQUIRED COMPONENTS $CLANG_LIBS)
Still not seeing how this finds Clang in thirdparty, given that there's no change to CMAKE_MODULE_PATH. How does the FATAL_ERROR below not trigger all the time?
http://gerrit.cloudera.org:8080/#/c/4475/1/build-support/tools/kudu-lint/README
File build-support/tools/kudu-lint/README:
PS1, Line 28: 3.8
> OK, I'll make a note that this tool isn't actively maintained and may not w
I think you forgot to make that note; this still references 3.8 explicitly (which is no longer the version in thirdparty).
http://gerrit.cloudera.org:8080/#/c/4475/2/build-support/tools/kudu-lint/README
File build-support/tools/kudu-lint/README:
PS2, Line 43: 8
Unrelated, but this should probably be $(nproc)
--
To view, visit http://gerrit.cloudera.org:8080/4475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c69be84cad56df71cc1bb2a3a89ada5d2167665
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
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] Update kudu-lint to LLVM 3.9
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has abandoned this change. ( http://gerrit.cloudera.org:8080/4475 )
Change subject: Update kudu-lint to LLVM 3.9
......................................................................
Abandoned
kudu-lint has fallen into disrepair. Apparently the kudu-specific checks weren't useful enough to merit the maintenance burden here, and perhaps simpler approaches like clang-query might be easier nowadays anyway.
--
To view, visit http://gerrit.cloudera.org:8080/4475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I7c69be84cad56df71cc1bb2a3a89ada5d2167665
Gerrit-Change-Number: 4475
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
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>
[kudu-CR] Update kudu-lint to LLVM 3.9
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4475
to look at the new patch set (#2).
Change subject: Update kudu-lint to LLVM 3.9
......................................................................
Update kudu-lint to LLVM 3.9
Also removed the 'unused Status' matcher, since this is doable using the
built-in WARN_UNUSED_RESULT attribute.
Change-Id: I7c69be84cad56df71cc1bb2a3a89ada5d2167665
---
M build-support/tools/kudu-lint/CMakeLists.txt
M build-support/tools/kudu-lint/README
D build-support/tools/kudu-lint/cmake_modules/FindClang.cmake
D build-support/tools/kudu-lint/cmake_modules/FindLLVM.cmake
M build-support/tools/kudu-lint/kudu-lint.cc
5 files changed, 23 insertions(+), 240 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/4475/2
--
To view, visit http://gerrit.cloudera.org:8080/4475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7c69be84cad56df71cc1bb2a3a89ada5d2167665
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
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>
[kudu-CR] Update kudu-lint to latest LLVM APIs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Update kudu-lint to latest LLVM APIs
......................................................................
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/4475/1/build-support/tools/kudu-lint/CMakeLists.txt
File build-support/tools/kudu-lint/CMakeLists.txt:
PS1, Line 22: # Find LLVM
This isn't actually getting LLVM/clang from thirdparty, is it? I think we'd need to alter CMAKE_MODULE_PATH for that to work.
PS1, Line 63: curses
: dl
: pthread
: z
Should we do some find_library() or find_package() calls for these?
PS1, Line 69: # Disable RTTI since we have to inherit from Clang-provided classes,
: # and Clang does not enable RTTI.
Maybe update this comment?
http://gerrit.cloudera.org:8080/#/c/4475/1/build-support/tools/kudu-lint/README
File build-support/tools/kudu-lint/README:
PS1, Line 28: 3.8
Nit: can we drop or reword this? Otherwise it's stale once clang in thirdparty is upgraded.
Line 43: $ find src -name \*.cc | xargs -n1 -P8 ./build-support/tools/kudu-lint/kudu-lint \
Shouldn't there be a make step after cmake? Or is the cmake step under "Running" somehow different than the one in "Building"?
PS1, Line 44: -extra-arg
Doesn't your change drop support for the -extra-arg argument?
--
To view, visit http://gerrit.cloudera.org:8080/4475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c69be84cad56df71cc1bb2a3a89ada5d2167665
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Update kudu-lint to latest LLVM APIs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Update kudu-lint to latest LLVM APIs
......................................................................
Patch Set 1:
We probably want to integrate tooling like this into clang-tidy as a "tidy check" in the longer run, but I happened to take a look at this last night and figured we may as well commit the fix, even if we later throw this code away.
--
To view, visit http://gerrit.cloudera.org:8080/4475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c69be84cad56df71cc1bb2a3a89ada5d2167665
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Update kudu-lint to latest LLVM APIs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Update kudu-lint to latest LLVM APIs
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4475/1/build-support/tools/kudu-lint/CMakeLists.txt
File build-support/tools/kudu-lint/CMakeLists.txt:
PS1, Line 22: # Find LLVM
> This isn't actually getting LLVM/clang from thirdparty, is it? I think we'd
You know, I'm not sure. It "just worked" on my machine and I dont really want to spend time to investigate it. At least this patch makes it more likely to work than it used to (which didn't work at all)
PS1, Line 63: curses
: dl
: pthread
: z
> Should we do some find_library() or find_package() calls for these?
I suppose so but again I dont think it's worth spending the time since this tool should probably go away.
http://gerrit.cloudera.org:8080/#/c/4475/1/build-support/tools/kudu-lint/README
File build-support/tools/kudu-lint/README:
PS1, Line 28: 3.8
> Nit: can we drop or reword this? Otherwise it's stale once clang in thirdpa
OK, I'll make a note that this tool isn't actively maintained and may not work with versions other than 3.8. (it seems to work with 3.9 for me as well though)
PS1, Line 44: -extra-arg
> Doesn't your change drop support for the -extra-arg argument?
nope, it's actually part of the standard tooling infrastructure now.
--
To view, visit http://gerrit.cloudera.org:8080/4475
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7c69be84cad56df71cc1bb2a3a89ada5d2167665
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes