[PATCH] D86379: [sanitizer] When building tests on Darwin, always pass -arch and other common flags

[Dan Liew via Phabricator via llvm-commits]

delcypher added inline comments.


================
Comment at: compiler-rt/cmake/config-ix.cmake:221
 macro(get_test_cc_for_arch arch cc_out cflags_out)
-  if(ANDROID OR ${arch} MATCHES "arm|aarch64")
+  if(NOT APPLE AND (ANDROID OR ${arch} MATCHES "arm|aarch64"))
     # This is only true if we are cross-compiling.
----------------
kubamracek wrote:
> delcypher wrote:
> > delcypher wrote:
> > > The CMake documentation (https://cmake.org/cmake/help/v3.1/command/if.html) makes it look like (I've not verified this) the operator precedence is
> > > 
> > > ```
> > > NOT (APPLE AND (ANDROID OR ${arch} MATCHES "arm|aarch64"))
> > > ```
> > > 
> > > which is wrong because the second condition has been flipped compared to what existed prior to this patch.
> > > 
> > > Please explicitly use parentheses here to remove any ambiguity.
> > > 
> > > I'm guessing you wanted:
> > > 
> > > ```
> > > (NOT APPLE) AND (ANDROID OR ${arch} MATCHES "arm|aarch64"))
> > > ```
> > Sorry I meant
> > 
> > ```
> > (NOT APPLE) AND (ANDROID OR ${arch} MATCHES "arm|aarch64")
> > ```
> No, "NOT" behaves sensibly in CMake, i.e. `if (NOT A AND B)` actually means `if ((NOT A) AND B)`. Given that (and the fact that we use this short form in many other places in compiler-rt CMake), do you still want me to add the parens?
Ah I read

```
if(NOT <expression>)
   True if the expression is not true.
```

and took that to mean that `NOT` would be evaluated last when used in an `if()` this way.


but yeah it seems that `NOT` has higher precedence than `AND` and `OR` when reading the text more carefully.

```
Parenthetical expressions are evaluated first followed by unary tests such as EXISTS, COMMAND, and DEFINED. Then any binary tests such as EQUAL, LESS, GREATER, STRLESS, STRGREATER, STREQUAL, and MATCHES will be evaluated. Then boolean NOT operators and finally boolean AND and then OR operators will be evaluated.
```

I checked manually and this precedence appears to be respected. So no need to change the parentheses here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86379