<div dir="ltr">On Wed, May 8, 2013 at 6:57 PM, Nick Kledzik <span dir="ltr"><<a href="mailto:kledzik@apple.com" target="_blank">kledzik@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>I'm happy with a separate line for each parameter with a trailing comment that Rui added.</div>
<div><br></div><div>But, another way to make this more readable is to create a SearchOptions struct and use it to hold the bool parameters. So the client code becomes:</div><div><br></div><div><span style="white-space:pre-wrap"> </span>InputFiles:SearchOptions options.</div>
<div><span style="white-space:pre-wrap"> </span>options.searchSharedLibs = true;</div><div><span style="white-space:pre-wrap"> </span>options.searchArchives = true;</div><div><span style="white-space:pre-wrap"> </span>options.dataSymbolOnly = false;</div>
<div><span style="white-space:pre-wrap"> </span>_inputFiles.searchLibraries(undefinedName, options, *this);</div></div></blockquote><div style><br></div><div style>Ah, I like that. I'm not sure if making another change worth doing, but that's more readable.</div>
<div style><br></div><div style>Rui</div><div style><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div></div><div>-Nick</div>
<div><div class="h5"><div><br></div><div><br></div><div><div>On May 8, 2013, at 6:28 PM, Rui Ueyama <<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>> wrote:</div><blockquote type="cite"><div style="letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div dir="ltr">I think LLD does not have another convention for comments on arguments. Added Michael to confirm.<div><br></div><div>I originally tried to make it a bitmask, but I didn't like the resulting code, because I needed to make a bitmask with code like this in this function:<br>
<br></div><div> (searchArchives ? SLK_Archives : 0) | (searchSharedLibs ? SLK_SharedLibs : 0) | SLK_dataSymbolOnly</div><div><br></div><div>The repeated (x ? y : 0) pattern looks too verbose too me, so I thought just adding comments would be better.</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, May 8, 2013 at 6:02 PM, Chandler Carruth<span> </span><span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span><span> </span>wrote:<br>
<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 dir="ltr"><div>On Thu, May 9, 2013 at 1:54 AM, Rui Ueyama<span> </span><span dir="ltr"><<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>></span><span> </span>wrote:<br>
</div><div class="gmail_extra"><div class="gmail_quote"><div><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">
Author: ruiu<br>Date: Wed May 8 18:54:10 2013<br>New Revision: 181492<br><br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project?rev=181492&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=181492&view=rev</a><br>
Log:<br>[lld] Add comments to InputFiles::searchLibraries() arguments.<br></blockquote><div><br></div></div><div>I'm not sure this is really the best approach, and if it is it would be better to follow the conventions for comments on arguments in the rest of LLVM (unless LLD has *another* divergent convention here??).</div>
<div><br></div><div>Specifically, could we instead use the (very common in LLVM and Clang) pattern of:</div><div><br></div><div>foo(/*Arg1=*/ true, /*Arg2=*/ false, ...);</div><div><br></div><div><br></div><div>Maybe even better would be to provide interfaces that don't require so many booleans. For example, here it seems like a bitmask enum would be nicely self-documenting?</div>
<div><br></div><div>_inputFiles.searchLibraries(undefName, SLK_SharedLibs | SLK_Archives | ...);</div><div><br></div><div>Just something to consider.</div><div><div><div> </div><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">
<br>Modified:<br> <span> </span>lld/trunk/lib/Core/Resolver.cpp<br><br>Modified: lld/trunk/lib/Core/Resolver.cpp<br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/lld/trunk/lib/Core/Resolver.cpp?rev=181492&r1=181491&r2=181492&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/lib/Core/Resolver.cpp?rev=181492&r1=181491&r2=181492&view=diff</a><br>
==============================================================================<br>--- lld/trunk/lib/Core/Resolver.cpp (original)<br>+++ lld/trunk/lib/Core/Resolver.cpp Wed May 8 18:54:10 2013<br>@@ -200,7 +200,11 @@ void Resolver::resolveUndefines() {<br>
StringRef undefName = undefAtom->name();<br> // load for previous undefine may also have loaded this undefine<br> if (!_symbolTable.isDefined(undefName)) {<br>- _inputFiles.searchLibraries(undefName, true, true, false, *this);<br>
+ _inputFiles.searchLibraries(undefName,<br>+ true, // searchSharedLibs<br>+ true, // searchArchives<br>+ false, // dataSymbolOnly<br>
+ *this);<br> }<br> }<br> // search libraries for overrides of common symbols<br>@@ -213,10 +217,13 @@ void Resolver::resolveUndefines() {<br> const Atom *curAtom = _symbolTable.findByName(tentDefName);<br>
assert(curAtom != nullptr);<br> if (const DefinedAtom* curDefAtom = dyn_cast<DefinedAtom>(curAtom)) {<br>- if (curDefAtom->merge() == DefinedAtom::mergeAsTentative ) {<br>+ if (curDefAtom->merge() == DefinedAtom::mergeAsTentative) {<br>
// Still tentative definition, so look for override.<br>- _inputFiles.searchLibraries(tentDefName, searchSharedLibs,<br>- searchArchives, true, *this);<br>+ _inputFiles.searchLibraries(tentDefName,<br>
+ searchSharedLibs,<br>+ searchArchives,<br>+ true, // dataSymbolOnly<br>+ *this);<br>
}<br> }<br> }<br><br><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>
</blockquote></div></div></div><br></div></div></blockquote></div><br></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></div></blockquote></div><br></div></div></div></blockquote></div><br></div></div>