Matthew Jacobs has posted comments on this change. Change subject: Support for building LLVM 3.7 and 3.8 on CentOS 5 ......................................................................
Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/2546/2/buildall.sh File buildall.sh: Line 60: with and without asserts It's not clear how this works. Having read through a bunch of code (and still being confused), I think that for version 3.3 there is some code that checks if the version is "3.3-no-asserts", then it compiles with --disable-assertions. Is that right? Can you maybe just indicate that the LLVM_VERSION string is what accomplishes that? Line 64: without asserts are we sure this is the case? I don't see us adding --disable-assertions anywhere except in the build-3.3.sh script. Also, maybe we should have a consistent naming scheme. http://gerrit.cloudera.org:8080/#/c/2546/2/source/llvm/build-source-tarball.sh File source/llvm/build-source-tarball.sh: Line 34: ../.. can we just use $THIS_DIR? Line 39: ../../../.. $THIS_DIR? Line 48: ../.. $THIS_DIR? Line 54: cd .. It's hard to reason about the relative directories. I think you're cd'ing to $THIS_DIR here? can we use that instead? In fact, maybe just remove the pushd on l31 (change to cd tools) then we can just cd to $THIS_DIR here. I think that's easier to reason about. -- To view, visit http://gerrit.cloudera.org:8080/2546 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iad649363fc64f30e0f17a9d51d7694a6fc00bc12 Gerrit-PatchSet: 2 Gerrit-Project: Toolchain Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
