[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