[PATCH] D40238: COFF: Do not create SectionChunks for discarded comdat sections.
Rui Ueyama via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 20 22:47:33 PST 2017
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
More information about the llvm-commits
mailing list