<div dir="ltr"><div class="gmail_extra"><div>On Tue, Aug 27, 2013 at 4:30 PM, Argyrios Kyrtzidis <span dir="ltr"><<a href="mailto:kyrtzidis@apple.com" target="_blank">kyrtzidis@apple.com</a>></span> wrote:<br></div><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><div class="h5">
<div>On Aug 27, 2013, at 4:10 PM, Michael Spencer <<a href="mailto:bigcheesegs@gmail.com" target="_blank">bigcheesegs@gmail.com</a>> wrote:</div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div>
On Tue, Aug 27, 2013 at 3:42 PM, Argyrios Kyrtzidis <span dir="ltr"><<a href="mailto:kyrtzidis@apple.com" target="_blank">kyrtzidis@apple.com</a>></span> wrote:<br></div><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><div>
<div>On Aug 27, 2013, at 3:36 PM, Michael Spencer <<a href="mailto:bigcheesegs@gmail.com" target="_blank">bigcheesegs@gmail.com</a>> wrote:</div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div>
On Wed, Aug 21, 2013 at 1:46 PM, Argyrios Kyrtzidis <span dir="ltr"><<a href="mailto:kyrtzidis@apple.com" target="_blank">kyrtzidis@apple.com</a>></span> wrote:<br></div><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word">Committed in r188921, thanks for reviewing!<div>
<div><br><div><div>On Aug 21, 2013, at 11:43 AM, Michael Gottesman <<a href="mailto:mgottesman@apple.com" target="_blank">mgottesman@apple.com</a>> wrote:</div><br><blockquote type="cite"><div style="word-wrap:break-word">
LGTM<div><br><div><div>On Aug 21, 2013, at 9:43 AM, Argyrios Kyrtzidis <<a href="mailto:kyrtzidis@apple.com" target="_blank">kyrtzidis@apple.com</a>> wrote:</div><br><blockquote type="cite"><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;word-wrap:break-word">
<div><div>On Aug 20, 2013, at 11:24 PM, Michael Gottesman <<a href="mailto:mgottesman@apple.com" target="_blank">mgottesman@apple.com</a>> wrote:</div><br><blockquote type="cite"><div style="word-wrap:break-word">Looks much better. One last nit then LGTM. I understand why you would make {add,ignore}_llvm_tool_subdirectory macros since they are so small,</div>
</blockquote><div><br></div><div>Actually these need to be macros so they can introduce the LLVM_IMPLICIT_PROJECT_IGNORE variable.</div><br><blockquote type="cite"><div style="word-wrap:break-word">but do you have any reason why you made add_llvm_implicit_external_projects a macro. It seems to me to be more of a function since it:<div>
<br></div><div>1. Takes no arguments.</div><div>2. Sets variables which are used only locally suggesting that you want to have the local scope of a function instead of the global scope of a macro.</div></div></blockquote>
<div><br></div><div>You're right; changed add_llvm_implicit_external_projects to a function and attached new patch.</div><div><br></div><div></div></div><span><auto-external-proj3.diff></span><div><div></div><div>
<br></div><div>-Argyrios</div><br><blockquote type="cite"><div style="word-wrap:break-word"><div><br></div><div>Michael<br><div><br><div><div>On Aug 20, 2013, at 6:00 PM, Argyrios Kyrtzidis <<a href="mailto:kyrtzidis@apple.com" target="_blank">kyrtzidis@apple.com</a>> wrote:</div>
<br><blockquote type="cite"><div style="font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
On Aug 20, 2013, at 5:27 PM, Michael Gottesman <<a href="mailto:mgottesman@apple.com" target="_blank">mgottesman@apple.com</a>> wrote:<br><br><blockquote type="cite">Hey Argyrios.<br><br>I like the patch, but I think that the name of the variable LLVM_TOOL_SUBDIRS is misleading since you are appending to it subdirectories that are going to explicitly not be included as well as those that will be. I can easily see a tired befuddled programmer getting confused about the meaning of said variable since it violates the expectations that every directory in that variable will include a tool that will be built (I was confused and I am only slightly tired/befuddled).<br>
<br>The real thing you are using said variable for is to act as a signal to add_llvm_implicit_external_projects that it should ignore said subdirectory. Perhaps something like LLVM_IMPLICIT_PROJECT_IGNORE? Even though that name sucks, it at least makes it absolutely clear about the purpose of the variable.<br>
</blockquote><br>Yes, that is better, thanks!<br><br>Attached new patch.<br><br><span><auto-external-proj2.diff></span></div></blockquote></div><br></div></div></div>_______________________________________________<br>
llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></blockquote>
</div></div></blockquote></div><br></div></div>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div></div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div><div class="gmail_extra">This breaks my usecase of not having lld located in the tools directory.</div></div></blockquote><div><br></div></div></div><div>Not quite sure what you mean, if the lld source is not located in tools, there should be no difference.</div>
<div>Could you elaborate ?</div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><br></div><div class="gmail_extra">- Michael Spencer<br></div>
</div>
</blockquote></div><br></div></blockquote><div><br></div><div>I have all of the LLVM subprojects laid out how they are in svn. So I have llvm-projects/{llvm,clang,lld,...}. Previously I would set LLVM_EXTERNAL_LLD_SOURCE_DIR to llvm-projects/lld and it would pick it up. Now since there is no call to add_llvm_external_project(lld), CMake never looks at LLVM_EXTERNAL_LLD_SOURCE_DIR.</div>
</div></div></div></blockquote><div><br></div></div></div><div>I see, please try again after r189415.</div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div> </div></div>- Michael Spencer<br></div></div><div class="im">
_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></blockquote></div><br></div>
</blockquote></div><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">Works. Thanks!</div><div class="gmail_extra"><br></div><div class="gmail_extra">- Michael Spencer<br></div></div>