[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