[PATCH] D132361: [LLD] [COFF] Fix export directives in object files from -includeoptional

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: lld/COFF/Driver.cpp:2194
   // converge.
-  do {
+  auto updateLinkLoop = [&] {
     // Windows specific -- if entry point is not found,
----------------
Personally, I think long lambdas can make it hard to figure out the structure of the code, so I'd like to try and avoid them if possible, and if necessary, make it a standalone function or method.


================
Comment at: lld/COFF/Driver.cpp:2241
   if (args.hasArg(OPT_include_optional)) {
     // Handle /includeoptional
     for (auto *arg : args.filtered(OPT_include_optional))
----------------
Can this not be moved into the body of the loop?


================
Comment at: lld/COFF/Driver.cpp:2251
   // Create wrapped symbols for -wrap option.
   std::vector<WrappedSymbol> wrapped = addWrappedSymbols(ctx, args);
   // Load more object files that might be needed for wrapped symbols.
----------------
I'm not sure if `addWrappedSymbols` can be called multiple times, but if it can, or if it can be made safe such that it is only done once, I'd suggest moving it into the loop as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132361