<div dir="ltr"><div>On Wed, Feb 22, 2017 at 1:50 AM, George Rimar via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br></div><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">grimar added inline comments.<br>
<span class=""><br>
<br>
================<br>
Comment at: lld/trunk/ELF/LinkerScript.<wbr>cpp:568<br>
   if (Cmd->LMAExpr)<br>
-    LMAOffset = Cmd->LMAExpr(Dot) - Dot;<br>
+    CurLMA = {Cmd->LMAExpr, Dot};<br>
   OutputSectionBase *Sec = findSection<ELFT>(Cmd->Name, *OutputSections);<br>
----------------<br>
</span><span class="">ruiu wrote:<br>
> If you change this to<br>
><br>
>   LMAOffset = [=] { return Cmd->LMAExpr(Dot) - Dot; }<br>
><br>
> I generally don't like to overuse std::pair because its member names (first and second) sometimes hurts code readability.<br>
</span>That would work if I do something like:<br>
<br>
```<br>
  if (Cmd->LMAExpr) {<br>
    uintX_t D = Dot;<br>
    LMAOffset = [=] { return Cmd->LMAExpr(D) - D; };<br>
  }<br>
```<br>
<br>
Because you can't capture member Dot by value.<br>
Does it look fine for you ?<br></blockquote><div><br></div><div>Yes, I think it's better. In C++14 you can write it as `[D= Dot] { return Cmd->LMAExpr(D) - D; }`, so there's an easy upgrade path forward. :)</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
<br>
================<br>
Comment at: lld/trunk/ELF/LinkerScript.h:<wbr>301<br>
   uintX_t Dot;<br>
-  uintX_t LMAOffset = 0;<br>
+  std::pair<Expr, uintX_t> CurLMA;<br>
   OutputSectionBase *CurOutSec = nullptr;<br>
----------------<br>
</span><span class="">ruiu wrote:<br>
> This can be just `Expr LMAOffset`, right?<br>
><br>
</span>I would use std::function<uint64_t()> then. Expr assumes Dot argument which is unused.<br>
And would complicate the initialization to:<br>
<br>
```<br>
LMAOffset = {[=](uint64_t) { return Cmd->LMAExpr(D) - D; }};<br>
```<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D30163" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D30163</a><br>
<br>
<br>
<br>
</div></div></blockquote></div><br></div></div>