[PATCH] D42716: [COFF] make /incremental control overwriting unchanged import libraries

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/COFF/Driver.cpp:697-698
 
+// Handle /incremental and emit warnings as necessary.
+static void handleIncremental(opt::InputArgList &Args) {
+  bool WarnIncompatible = true;
----------------
Usually we define get<OptionName> (e.g. getIncremental) which returns a boolean value, so you can write

  Config->Incremental = getIncremental(Args);

in LinkerDriver::link.

Then after parsing all options, we do check option incompatibility like this

  if (Config->Incremental && Config->DoICF) {
    warn("ignoring /INCREMENTAL due to /icf specification");
    return;
  }

  if (Config->Incremental && Config->DoGC) {
    warn("ignoring /INCREMENTAL due to /ref specification");
    Config->Incremental = false;
  }

  if (Config->Incremental && !Config->Order.empty()) {
    warn("ignoring /INCREMENTAL due to /order specification");
    Config->Incremental = false;
  }

This is slightly lengthy than the code that uses a lambda, but this is perhaps easier to read.



================
Comment at: lld/COFF/Driver.cpp:1036-1046
+  // There is a complex interaction with /incremental. Incremental linking is
+  // incompatible with /opt:icf, and /opt:ref. If you specify both /incremental
+  // and one of those options, you get a warning and incremental linking is
+  // turned off. If you don't explicitly specify /incremental, it is enabled by
+  // default, but turned off without a warning if one of the incompatible
+  // options is specified. Conversely, if you specify /incremental, the defaults
+  // for /opt:icf and /opt:ref are changed to be compatible with it.
----------------
I don't think we actually have to mimic this behavior. If you have a real incremental linking you have to do this because otherwise it is very hard to do incremental linking, but that's an artifact of the implementation. Can we just keep the original code?


================
Comment at: lld/COFF/Options.td:107-108
+defm incremental : B<"incremental",
+                     "Link incrementally",
+                     "Perform a full link">;
 defm largeaddressaware : B<"largeaddressaware",
----------------
This description is misleading as we don't actually do incremental linking.


https://reviews.llvm.org/D42716