[PATCH] D109634: [LLD] Remove global state in lld/COFF

[Adrian McCarthy via Phabricator via llvm-commits]

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