[PATCH] D113866: [LLD] [COFF] Omit IMAGE_SYM_CLASS_LABEL symbols from the PE symbol table

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo marked 2 inline comments as done.
mstorsjo added inline comments.


================
Comment at: lld/COFF/Writer.cpp:1213-1214
           continue;
+        auto *dc = dyn_cast_or_null<DefinedCOFF>(d);
+        if (dc) {
+          COFFSymbolRef symRef = dc->getCOFFSymbol();
----------------
rnk wrote:
> If `dc` will only be used in the `if`, it's preferred to declare the variable inside the if condition.
Sure, thanks.


================
Comment at: lld/COFF/Writer.cpp:1220
+        }
         d->writtenToSymtab = true;
 
----------------
rnk wrote:
> Please keep this grouped with the check, so it's clear that it's one operation to ensure that each symbol is written at  most once.
Ok, will do. I was a bit divided over this while writing the initial version, as it also seems counterintuitive to set a flag "written" while we might still bail out and not write it. But setting the flag avoids needlessly checking the storage class multiple times (and keeps the flag check more together).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113866