[PATCH] D40998: [driver][darwin] Take the OS version specified in "-target" as the target OS instead of inferring it from SDK / environment

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith requested changes to this revision.
dexonsmith added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Driver/ToolChains/Darwin.cpp:1234-1276
+    // The -target option specifies the deployment target when
+    // -m<os>-version-min is not given and the OS version is present in the
+    // target.
+    if (Args.hasArg(options::OPT_target) && getTriple().getOSMajorVersion() &&
+        getTriple().isOSDarwin()) {
+      unsigned Major, Minor, Micro;
+      auto RewriteVersionMinArg = [&](options::ID O) -> Arg * {
----------------
This logic seems to be duplicated from the `-target` handling below.  Perhaps this would be simpler if `-target` were processed first.

I suggest the following refactoring in an NFC prep patch:
- First, check for a `-target` option.  If it exists, extract all the bits.
- Then, handle the logic for `-arch`, `-m*-version-min`, and `-isysroot` (matching the current semantics, which I think are "override `-target`").

In a follow-up patch, change `-isysroot`/`SDKROOT` not to override a `-target`-specified environment (the semantic change from this patch).  After the refactoring, I suspect this will be trivial.

We should also warn/error when `-arch` disagrees with `-target`, and likely the same for `-m*-version-min`.  I suspect these follow-ups will also be trivial.


================
Comment at: lib/Driver/ToolChains/Darwin.cpp:1273
+      default:
+        llvm_unreachable("invalid Os");
+      }
----------------
I'd spell this `"invalid OS"`.


Repository:
  rC Clang

https://reviews.llvm.org/D40998