[PATCH] D27415: [ELF] - Replace MergeOutputSection with synthetic input section MergeSection.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/InputSection.cpp:134
+  case Merge: {
+    const MergeInputSection<ELFT> *MS = cast<MergeInputSection<ELFT>>(this);
+    if (!MS->Live)
----------------
Use auto.


================
Comment at: ELF/OutputSections.cpp:133
+
+  auto *SS = dyn_cast<SyntheticSection<ELFT>>(C);
+  if (SS && SS->Mergeable)
----------------
Can you keep the scope as narrow as possible? `SS` has unnecessarily large scope here. If you do this, it'll be much narrower.

  if (auto *Sec = dyn_cast<...>())
    if (Sec->Mergeable)
      ...;


================
Comment at: ELF/OutputSections.cpp:135
+  if (SS && SS->Mergeable)
+    static_cast<MergeSection<ELFT> *>(SS)->setOutputSection(this);
+
----------------
You seems to be reinventing dynamic class dispatching. Please define dyn_cast<> for MergeSection<ELFT>.


================
Comment at: ELF/Writer.cpp:1728
+
+template void elf::combineMergableSections(std::vector<InputSectionBase<ELF32LE> *> &V);
+template void elf::combineMergableSections(std::vector<InputSectionBase<ELF32BE> *> &V);
----------------
Remove `V`.


https://reviews.llvm.org/D27415