[PATCH] [ELF] Refactor File.h.

[Shankar Kalpathi Easwaran]

================
Comment at: lib/ReaderWriter/ELF/File.h:413-415
@@ -444,33 +412,5 @@
 
-          // If this is the last atom, lets not create a followon
-          // reference
-          if ((si + 1) != se)
-            anonFollowedBy = new (_readerStorage)
-               ELFReference<ELFT>(lld::Reference::kindLayoutAfter);
-          anonPrecededBy = new (_readerStorage)
-              ELFReference<ELFT>(lld::Reference::kindLayoutBefore);
-          // Add the references to the anonymous atom that we created
-          if (anonFollowedBy)
-            anonAtom->addReference(anonFollowedBy);
-          anonAtom->addReference(anonPrecededBy);
-          if (previous_atom)
-            anonPrecededBy->setTarget(previous_atom);
-          contentSize = 0;
-        }
-
-        ArrayRef<uint8_t> symbolData = ArrayRef<uint8_t>(
-            (uint8_t *)sectionContents.data() + (*si)->st_value, contentSize);
-
-        // If the linker finds that a section has global atoms that are in a
-        // mergeable section, treat them as defined atoms as they shouldnt be
-        // merged away as well as these symbols have to be part of symbol
-        // resolution
-        if (isMergeableSection(section)) {
-          if ((*si)->getBinding() == llvm::ELF::STB_GLOBAL) {
-            auto definedMergeAtom = new (_readerStorage) ELFDefinedAtom<ELFT>(
-                *this, symbolName, sectionName, (*si), section, symbolData,
-                _references.size(), _references.size(), _references);
-            _definedAtoms._atoms.push_back(definedMergeAtom);
-          }
-          continue;
+          if (previousAtom)
+            createEdge(anonAtom, previousAtom,
+                       lld::Reference::kindLayoutBefore);
         }
----------------
Rui Ueyama wrote:
> Shankar Kalpathi Easwaran wrote:
> > You can possibly check if the second parameter is null and return from createEdge.
> Yeah, if I make such change I would rename createEdge -> maybeCreateEdge. I think this is a matter of taste, but I feel like for a function that creates an edge, the precondition is the existence of nodes. My preference is to keep this null check outside of the function and keep the function as well defined as possible.
?

================
Comment at: lib/ReaderWriter/ELF/File.h:403-407
@@ -474,5 +402,7 @@
+        if (isMergeableStringSection(section)) {
+          if (symbol->getBinding() == llvm::ELF::STB_GLOBAL) {
             auto definedMergeAtom = new (_readerStorage) ELFDefinedAtom<ELFT>(
                 *this, symbolName, sectionName, (*si), section, symbolData,
                 _references.size(), _references.size(), _references);
             _definedAtoms._atoms.push_back(definedMergeAtom);
           }
----------------
can you add a test if the current tests are not covering this case ?

================
Comment at: lib/ReaderWriter/ELF/File.h:235
@@ -235,3 +234,3 @@
     std::vector<MergeString *> tokens;
-    for (auto msi : _mergeStringSections) {
+    for (const Elf_Shdr *msi : _mergeStringSections) {
       StringRef sectionContents;
----------------
Rui Ueyama wrote:
> Shankar Kalpathi Easwaran wrote:
> > why is this change from auto to const Elf_Shdr ?
> Because I think this is more readable. _mergeStringSection is not defined in this file, and it's not easy to get its type at first sight. The type is the same.
?


http://llvm-reviews.chandlerc.com/D921