[PATCH] [lld][LayoutPass] Split buildFollowOnTable for readability.

Rui Ueyama ruiu at google.com
Mon May 13 17:28:29 PDT 2013



================
Comment at: include/lld/Passes/LayoutPass.h:76-77
@@ +75,4 @@
+  // Helper methods for buildFollowOnTable().
+  const DefinedAtom *findAtomFollowedBy(const DefinedAtom *targetAtom);
+  bool checkAllPrevAtomsZeroSize(const DefinedAtom *targetAtom);
+
----------------
Shankar Kalpathi Easwaran wrote:
> These should be const functions as well.
Right. But because there's no const-qualified operator[] is defined for DenseMap, it can't compile if we add const.

================
Comment at: lib/Passes/LayoutPass.cpp:136
@@ +135,3 @@
+      return prevAtom;
+  }
+}
----------------
Shankar Kalpathi Easwaran wrote:
> This should return a nullptr if nothing is found.
This function should return an atom for any input, or there's a bug in this file. We have an assertion to catch such an error.

================
Comment at: lib/Passes/LayoutPass.cpp:153-157
@@ +152,7 @@
+      break;
+    if ((*atom).size() == 0) {
+      // TODO: print warning that an impossible layout is being desired by the
+      // user.
+      return false;
+    }
+  }
----------------
Shankar Kalpathi Easwaran wrote:
> remove the extra braces
Done.

================
Comment at: lib/Passes/LayoutPass.cpp:170-171
@@ +169,4 @@
+        _followOnNexts.find(targetAtom);
+    if (targetFollowOnAtomsIter == _followOnNexts.end())
+      break;
+    targetAtom = targetFollowOnAtomsIter->second;
----------------
Shankar Kalpathi Easwaran wrote:
> could just return here.
Done.

================
Comment at: lib/Passes/LayoutPass.cpp:391-393
@@ -420,4 +390,5 @@
     if (start != _followOnRoots.end()) {
-      for (const DefinedAtom *nextAtom = start->second; nextAtom != NULL;
+      for (const DefinedAtom *nextAtom = start->second;
+           nextAtom;
            nextAtom = _followOnNexts[nextAtom]) {
         AtomToOrdinalT::iterator pos = _ordinalOverrideMap.find(nextAtom);
----------------
Shankar Kalpathi Easwaran wrote:
> you might want to run the changes with clang-format, if it has not been done already. 
This would become a pretty large change if I run clang-format, so I won't do that at least in this patch. We might want to do that in another patch.


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



More information about the llvm-commits mailing list