<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sun, Mar 8, 2015 at 7:57 PM, Shankar Kalpathi Easwaran <span dir="ltr"><<a href="mailto:shankarke@gmail.com" target="_blank">shankarke@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">================<br>
Comment at: include/lld/ReaderWriter/ELFLinkingContext.h:304-306<br>
@@ -303,1 +303,5 @@<br>
<br>
+  const std::vector<std::unique_ptr<script::Parser>> &scripts() const {<br>
+    return _scripts;<br>
+  }<br>
+<br>
----------------<br>
do you want to add a boolean hasLinkerScript that checks the size and returns ?<br>
<br>
================<br>
Comment at: include/lld/ReaderWriter/LinkerScript.h:366<br>
@@ +365,3 @@<br>
+  // here is an overkill.<br>
+  typedef llvm::StringMap<int64_t, llvm::BumpPtrAllocator> SymbolTableTy;<br>
+<br>
----------------<br>
you could replace this with a llvm::DenseMap<StringRef, int64_t> and use a common StringRefMappingInfo.<br></blockquote><div><br></div><div>Just use std::map<std::string, T> if this is not in a performance critical path. Most "optimization" doesn't affect anything to the actual linker performance. 9 out of 10 cases simplicity is more important than marginal performance gain.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
================<br>
Comment at: include/lld/ReaderWriter/LinkerScript.h:794-795<br>
@@ -767,2 +793,4 @@<br>
   void dump(raw_ostream &os) const override;<br>
+  const_iterator begin() const { return _sectionsCommands.begin(); }<br>
+  const_iterator end() const { return _sectionsCommands.end(); }<br>
<br>
----------------<br>
Use lld::range,once you replace it with range, you can remove the typedef for const_iterator.<br>
<br>
================<br>
Comment at: lib/ReaderWriter/LinkerScript.cpp:581<br>
@@ +580,3 @@<br>
+  switch (_op) {<br>
<span>+  case Unary::Minus:<br>
+    return 0 - childRes;<br>
</span>----------------<br>
Do you want to handle Unary::Plus ??<br>
<br>
================<br>
Comment at: lib/ReaderWriter/LinkerScript.cpp:584<br>
@@ +583,3 @@<br>
+  case Unary::Not:<br>
+    return ~childRes;<br>
+  }<br>
----------------<br>
I thought linker script uses only a boolean value of a Unary Not, no ??<br>
<br>
================<br>
Comment at: lib/ReaderWriter/LinkerScript.cpp:703<br>
@@ +702,3 @@<br>
+    return trueExprRes;<br>
+  else<br>
+    return falseExprRes;<br>
----------------<br>
no need of else.<br>
<br>
================<br>
Comment at: unittests/DriverTests/GnuLdDriverTest.cpp:262<br>
@@ +261,3 @@<br>
+  EXPECT_EQ(script::SymbolAssignment::Simple, sa1->assignmentKind());<br>
+  EXPECT_EQ(script::SymbolAssignment::Normal, sa1->assignmentVisibility());<br>
+<br>
----------------<br>
Can we change SymbolAssignment::Normal to SymbolAssignment::Default ?<br>
<br>
Looks like you are using the same SymboAssignment to name two different things, assignmentKind and assignmentVisibilty. I would recommend separating them.<br>
<div><div><br>
<a href="http://reviews.llvm.org/D8156" target="_blank">http://reviews.llvm.org/D8156</a><br>
<br>
EMAIL PREFERENCES<br>
  <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br>
</div></div></blockquote></div><br></div></div>