[PATCH] D18499: [ELF] - Implemented prototype of location counter support.

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/LinkerScript.cpp:51-65
@@ +50,17 @@
+
+    StringRef Tok = Tokens[I];
+    if (Tok == ".") {
+      Result += LocCounter;
+      continue;
+    }
+
+    uint64_t Val;
+    if (Tok.getAsInteger(0, Val)) {
+      error("undeclared variable " + Tok);
+      return 0;
+    }
+
+    Result += Val;
+  }
+  return Result;
+}
----------------
ruiu wrote:
> Define this method
> 
>   uint64_t getInteger(StringRef S) {
>     uint64_t V;
>     if (S.getAsInteger(0, V)) {
>       setError("malformed number: " + V);
>       return 0;
>    }
>     retrurn V;
>   }
> 
> Then you can simplify this to
> 
>   StringRef Tok = Tokens[I];
>   if (Tok == ".")
>     Result += LocCounter;
>   else
>     Result += getInteger(Tok);
Done.
Btw, few iterations ago I had code like this.
http://reviews.llvm.org/D18499?id=52399#inline-156527
Looks when you requested to make a single function from whole class I understood this too straightforward (combined exactly into one function) :)

================
Comment at: ELF/LinkerScript.cpp:94
@@ +93,3 @@
+void LinkerScript::setSectionsAddresses(
+    llvm::StringMap<OutputSectionBase<ELFT> *> &S,
+    std::vector<LocationNode> &Loc) {
----------------
ruiu wrote:
> Are you aware of the fact that StringMap copies keys? Maybe DenseMap is more efficient because it doesn't copy StringRefs?
I missed that fact. Thanks for pointing on that.
There is no such method and variable anymore.

================
Comment at: ELF/LinkerScript.cpp:113
@@ +112,3 @@
+    uintX_t Align = Sec->getAlign();
+    if (Sec->getFlags() & SHF_TLS && Sec->getType() == SHT_NOBITS) {
+      uintX_t TVA = VA + ThreadBssOffset;
----------------
ruiu wrote:
> Needs () around &.
Done.

================
Comment at: ELF/LinkerScript.cpp:131
@@ +130,3 @@
+template <class ELFT>
+void LinkerScript::assignAddresses(std::vector<OutputSectionBase<ELFT> *> &S) {
+  typedef typename ELFT::uint uintX_t;
----------------
ruiu wrote:
> This function seems a bit too complicated. This function construct temporary data to feed to setSectionsAddresses. Do we really need to do that? Isn't there any way to make it simpler?
Reimplemented in much more straighforward way.

================
Comment at: ELF/LinkerScript.cpp:154
@@ +153,3 @@
+  for (OutputSectionBase<ELFT> *Sec : S)
+    if (NameToSec.count(Sec->getName()) == 0) {
+      StringRef Name = Sec->getName();
----------------
ruiu wrote:
> Flip the condition and use early continue.
There is no such code anymore.


http://reviews.llvm.org/D18499