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 <t...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to