Tim Armstrong 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 sti
Done.


Line 64: without asserts
> are we sure this is the case? I don't see us adding --disable-assertions an
The LLVM build scripts changed a lot between 3.3 and 3.7. We build 3.7 in 
Release mode, which disables assertions:

http://llvm.org/docs/HowToReleaseLLVM.html#building-the-release

A consistent naming scheme would be great, but the way the toolchain was 
originally set up 3.3 was built with asserts and 3.7 was built without, I think 
purely by mistake. We can be consistent going forward, but I can't 
retroactively fix the 3.3 naming without causing problems.

Anyway, I added a comment to make it a little clearer.


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?
Done


Line 39: ../../../..
> $THIS_DIR?
Done


Line 48: ../..
> $THIS_DIR?
Done


Line 54: cd ..
> It's hard to reason about the relative directories. I think you're cd'ing t
I tried to clean this up.


-- 
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

Reply via email to