<div dir="ltr">Hi Justin,<div><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">
The constructors are templated, but the member hardcodes 4096 so the<br>
only valid argument to the templates is 4096. Either template the type<br>
or un-template the constructors?<br></blockquote><div><br></div><div>Huh. I had missed that we couldn't even move-construct from other SmallVectors where the size differed.</div><div><br></div><div>I've submitted a patch to llvm-commits to add the necessary operations to SmallVector so that I can just pass a SmallVectorImpl<T>&&.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> +  PersistentMangler(DataLayout DL) : DL(std::move(DL)), M(&this->DL) {}<br>
<br>
</span>Shouldn't DL be an rvalue reference here?<br></blockquote><div><br></div><div>I usually just use pass-by-value where I can. You do end up doubling the number of move operations for clients who want to move, but that should still be cheap.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> +++ include/llvm/ExecutionEngine/Orc/LazyEmittingLayer.h      (working copy)<br>
 ...<br>
<span class="">> +    // Build the MangledNames map. Bails out early (with MangledNames set to<br>
> +    // nullptr) if the given SearchName is found while building the map.<br>
> +    void buildMangledNames(StringRef SearchName,<br></span><br>
This actually leaves MangledNames untouched if it bails out early. I<br>
don't know that it matters much which behaviour we choose, but please<br>
make the comment and the code match.<br></blockquote><div><br></div><div>Comment fixed. Thanks!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +++ lib/ExecutionEngine/Orc/IndirectionUtils.cpp      (working copy)<br>
 ...<br>
> +std::vector<std::unique_ptr<Module>><br>
> +explode(const Module &OrigMod, const JITIndirections &Indirections) {<br>
> +  std::set<std::string> ImplNames;<br>
> +<br>
> +  for (const auto &FuncName : Indirections.IndirectedNames)<br>
> +    ImplNames.insert(Indirections.GetImplName(FuncName));<br>
> +<br>
> +  return explode(<br>
> +      OrigMod, [&](const Function &F) { return ImplNames.count(F.getName()); });<br>
<br>
It's better to sort a vector and binary search here, rather than use a<br>
std::set.<br>
</blockquote></div><br></div></div><div class="gmail_extra">Good point. I'm going to leave this as is for now though: explode is probably going to get completely rewritten soon.</div><div class="gmail_extra"><br></div><div class="gmail_extra">Thanks for the review Justin!</div><div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">- Lang.</div></div>