[PATCH] D109634: [LLD] Remove global state in lld/COFF
Adrian McCarthy via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 10 15:43:38 PDT 2021
amccarth added a comment.
Minor suggestions, but otherwise LGTM. I assume you've coordinated with @aganea to ensure you aren't duplicating effort.
================
Comment at: lld/COFF/COFFLinkerContext.cpp:26
+ typeServerSourceMappings.clear();
+ precompSourceMappings.clear();
+}
----------------
I understand the need for `clearGHashes`, but clearing the other containers appears unnecessary. I wouldn't bother to explicitly clear them. Their destructors are going to do that.
================
Comment at: lld/COFF/DebugTypes.cpp:934
/// Get the ghash key for this cell.
- GloballyHashedType getGHash() const {
- return TpiSource::instances[getTpiSrcIdx()]->ghashes[getGHashIdx()];
+ GloballyHashedType getGHash(std::vector<TpiSource *> instances) const {
+ return instances[getTpiSrcIdx()]->ghashes[getGHashIdx()];
----------------
It looks like you're passing the entire vector in. I think you meant to pass it by reference (probably even const reference):
`const std::vector<TpiSource *> &instances`
================
Comment at: lld/COFF/Driver.cpp:75
freeArena();
- ObjFile::instances.clear();
- PDBInputFile::instances.clear();
- ImportFile::instances.clear();
- BitcodeFile::instances.clear();
- memset(MergeChunk::instances, 0, sizeof(MergeChunk::instances));
- OutputSection::clear();
+ // memset(MergeChunk::instances, 0, sizeof(MergeChunk::instances));
};
----------------
Consider deleting the memset rather than commenting it out.
================
Comment at: lld/COFF/Driver.cpp:952
// For some reason, the MSVC linker requires a filename to be
// preceded by "@".
if (!arg.startswith("@")) {
----------------
Heh. I know why. But it's unimportant nowadays.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109634/new/
https://reviews.llvm.org/D109634
More information about the llvm-commits
mailing list