[PATCH] D133273: [compiler-rt][macOS]: Fix building compiler-rt without iOS related SDKs

[Dan Liew via Phabricator via llvm-commits]

delcypher requested changes to this revision.
delcypher added a comment.
This revision now requires changes to proceed.

@thieta Thanks for working on this. I don't think can land as is because I'm not convinced this works correctly. Please see my comments.



================
Comment at: compiler-rt/cmake/base-config-ix.cmake:154
+  find_darwin_sdk_dir(HAS_IOS_SDK "iphoneos")
+  set(COMPILER_RT_ENABLE_IOS_DEFAULT ${DEFAULT})
+  if("${HAS_IOS_SDK}" STREQUAL "")
----------------
What is this `${DEFAULT}` variable? Shouldn't this just be:

```
set(COMPILER_RT_ENABLE_IOS_DEFAULT ON)
```

?


================
Comment at: compiler-rt/cmake/base-config-ix.cmake:158
+  endif()
+  option(COMPILER_RT_ENABLE_IOS_DEFAULT "Enable building for iOS" COMPILER_RT_ENABLE_IOS_DEFAULT)
+
----------------
Should this be

```
option(COMPILER_RT_ENABLE_IOS_DEFAULT "Enable building for iOS" ${COMPILER_RT_ENABLE_IOS_DEFAULT})
```

?

I.e.: Explicitly expand the variable?

Although `if()` will implicitly expand variables as noted [[ https://cmake.org/cmake/help/latest/manual/cmake-language.7.html#variable-references | here ]] I'm not aware of such a behavior for `option()`. Even if this was supported I think we should explicitly expand the variables as is done elsewhere in this source file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133273