[PATCH] D40238: COFF: Do not create SectionChunks for discarded comdat sections.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/COFF/InputFiles.cpp:142
     else
-      Chunks.push_back(C);
+      readSection(I, nullptr);
+  }
----------------
It seems a bit odd that you ignored a return value here. Turns out that readSection mutates SparseChunks as a side effect. But can you remove that assignment from that function and instead doing it explicitly in this line?


================
Comment at: lld/COFF/InputFiles.cpp:214
+  SectionChunk *Parent = SparseChunks[Def->getNumber(Sym.isBigObj())];
+  if (Parent == nullptr)
+    SparseChunks[SectionNumber] = nullptr;
----------------
I'd return early if Parent is PendingComdat.


================
Comment at: lld/COFF/InputFiles.cpp:222
+Symbol *ObjFile::createRegular(COFFSymbolRef Sym) {
+  auto *SC = SparseChunks[Sym.getSectionNumber()];
+  if (Sym.isExternal()) {
----------------
Replace `auto` with its actual type.


================
Comment at: lld/COFF/InputFiles.cpp:276
+    COFFSymbolRef Sym = check(COFFObj->getSymbol(I));
+    readSectionDefinition(Sym);
+    Symbols[I] = createRegular(Sym);
----------------
Do you think you can make readSectionDefinition to have fewer side effects? Not always using its return value seems a bit odd.


================
Comment at: lld/COFF/InputFiles.h:148
+              const llvm::object::coff_aux_section_definition *Aux);
+  const llvm::object::coff_aux_section_definition *
+  readSectionDefinition(COFFSymbolRef COFFSym);
----------------
Can you add a blank line after a multi-line declaration?


================
Comment at: lld/COFF/SymbolTable.cpp:241
+    replaceSymbol<DefinedRegular>(S, F, N, /*IsCOMDAT*/ true,
+                                  /*IsExternal*/ true, Sym, nullptr);
+  } else if (!cast<DefinedRegular>(S)->isCOMDAT())
----------------
Maybe just returning `{S, true}` and `{S, false}` is easier to read than using `Prevailing` variable.


================
Comment at: lld/COFF/SymbolTable.cpp:242
+                                  /*IsExternal*/ true, Sym, nullptr);
+  } else if (!cast<DefinedRegular>(S)->isCOMDAT())
+    reportDuplicate(S, F);
----------------
nit: add {}


https://reviews.llvm.org/D40238