[PATCH] D30507: Remove DefinedSynthetic

Rui Ueyama via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 8 10:05:15 PST 2017


ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

Let's try this one. LGTM. Please submit after addressing these minor comments.



================
Comment at: ELF/InputSection.cpp:107
+    auto *OS = cast<OutputSection>(this);
+    return Offset == uint64_t(-1) ? OS->Size : Offset;
+  }
----------------
Nice to add a comment saying that -1 indicates the end of the section.


================
Comment at: ELF/InputSection.cpp:135-136
     return EH->EHSec->OutSec;
-  return OutSec;
+  if (auto *IS = dyn_cast<InputSection>(this))
+    return IS->OutSec;
+  return cast<OutputSection>(this);
----------------
I would add this at the beginning of this function because this is the most common case.


================
Comment at: ELF/InputSection.h:37
 
-// This corresponds to a section of an input file.
-class InputSectionBase {
+class SectionBase {
 public:
----------------
Please add a comment -- this class represents an ELF section, either it is an input section or an output section.


================
Comment at: ELF/InputSection.h:39
 public:
-  enum Kind { Regular, EHFrame, Merge, Synthetic, };
+  enum Kind { Regular, EHFrame, Merge, Synthetic, Output };
 
----------------
I think `Output` has a risk of name conflict. Maybe it should be OutputKind, though adding "Kind" to all five enums should be done in another patch.


================
Comment at: ELF/InputSection.h:79
+              uint32_t Info, uint32_t Link)
+      : Name(Name), SectionKind(SectionKind), Alignment(Alignment),
+        Flags(Flags), Entsize(Entsize), Type(Type), Link(Link), Info(Info) {}
----------------
Maybe we should initialize Live and Assigned bits too so that even if there's a bug we get a consistent behavior.


https://reviews.llvm.org/D30507





More information about the llvm-commits mailing list