[PATCH] D71711: [COFF] Make the autogenerated .weak.<name>.default symbols static

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/lib/MC/WinCOFFObjectWriter.cpp:368
   COFFSymbol *Local = nullptr;
   if (cast<MCSymbolCOFF>(MCSym).isWeakExternal()) {
     Sym->Data.StorageClass = COFF::IMAGE_SYM_CLASS_WEAK_EXTERNAL;
----------------
While here, I think it would be better to hoist the cast into the entry block, rename the parameter to MCSymGeneric, and have a new local variable `const auto &MCSym = cast<MCSymbolCOFF>(MCSymGeneric);`


================
Comment at: llvm/lib/MC/WinCOFFObjectWriter.cpp:379
         WeakDefault->Section = Sec;
       Local = WeakDefault;
     }
----------------
I think we can assign static storage here.


================
Comment at: llvm/lib/MC/WinCOFFObjectWriter.cpp:396
       Sym->Section = Sec;
     Local = Sym;
   }
----------------
So, we could move the old storage class calculation here.


================
Comment at: llvm/lib/MC/WinCOFFObjectWriter.cpp:417
+    // default symbols for the same weak in other translation units.
+    if (SymbolCOFF.isWeakExternal())
+      Local->Data.StorageClass = COFF::IMAGE_SYM_CLASS_STATIC;
----------------
Can this be factored so we only check for weak externals once? We do it above and assign it to Local. All this StorageClass code is dead for the weak external case, so we can hoist this new StorageClass up to that block, and hoist the old StorageClass code into the else block.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71711/new/

https://reviews.llvm.org/D71711