<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Mar 28, 2014 at 10:14 AM, Shankar Easwaran <span dir="ltr"><<a href="mailto:shankare@codeaurora.org" target="_blank">shankare@codeaurora.org</a>></span> wrote:<br>


<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  
    
  
  <div text="#000000" bgcolor="#FFFFFF"><div>
    <div>On 3/28/2014 11:57 AM, Rui Ueyama
      wrote:<br>
    </div>
    </div><blockquote type="cite"><div>
      <pre>On Fri, Mar 28, 2014 at 9:53 AM, Rui Ueyama <a href="mailto:ruiu@google.com" target="_blank"><ruiu@google.com></a> wrote:

</pre>
      <blockquote type="cite">
        <pre>On Fri, Mar 28, 2014 at 9:42 AM, Shankar Kalpathi Easwaran <
<a href="mailto:shankarke@gmail.com" target="_blank">shankarke@gmail.com</a>> wrote:

</pre>
        <blockquote type="cite">
          <pre>  Ok, I agree about that its more complex to have the complete --defsym
option.

  Once the above comment is fixed, this patch LGTM.


================
Comment at: lib/Core/LinkingContext.cpp:59-61
@@ -58,5 +58,5 @@

-std::unique_ptr<File> LinkingContext::createUndefinedSymbolFile() const {
-  return createUndefinedSymbolFile("command line option -u");
+std::unique_ptr<File> LinkingContext::createCommandLineFile() const {
+  return createCommandLineFile("<command line option -u or --defsym>");
 }

----------------
Can we use a separate string for -u and --defsym. We could have a
createCommandLineFile with a StringRef like how PECOFF does, so that we can
support more command line defined by various options.
</pre>
        </blockquote>
        <pre>It's actually pretty odd that we have a piece of code in LinkingContext
that creates what seems ELF-specific thing ("<command line option -u or
--defsym>"). Need to be refactored. Anyways I'll create a separate file for
--defsym.

</pre>
      </blockquote>
      </div><pre>It turned out that it's not that easy -- each *Flavor*LinkingContext is
expected to create at most one file for its internal use. If it doesn't
need one, createCommandLIneFile return a nullptr, or a file instance
otherwise. That's working well, so we should keep it. Let's go with a
single file here.

</pre>
    </blockquote>
    <br>
    createInternalFiles would create a separate --defsym file and add it
    to the list of internal files, and append to the list of internal
    files.<br></div></blockquote><div><br></div><div>I don't see the benefit of having many files instead of one unified virtual file that driver creates. Maybe it's sometimes useful for debugging? I don't have strong opinion on this, so I'll create a separate file and add it in createInternalFiles for now.</div>


<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF">
    -<br>
    <br>
    Shankar Easwaran<br>
    <br>
    <br>
    <pre></pre>
    <blockquote type="cite">
      <pre></pre>
      <br>
      <fieldset></fieldset>
      <br>
      <pre>_______________________________________________
llvm-commits mailing list
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><span><font color="#888888">
</font></span></pre><span><font color="#888888">
    </font></span></blockquote><span><font color="#888888">
    <br>
    <br>
    <pre cols="72">-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation</pre>
  </font></span></div>

</blockquote></div><br></div></div>