[PATCH] D20558: [LibFuzzer] Start cleaning up the CMakeLists.txt files.

[Dan Liew via llvm-commits]

delcypher added inline comments.

================
Comment at: lib/Fuzzer/CMakeLists.txt:10
@@ +9,3 @@
+    "-fno-sanitize=all"
+    # FIXME: gcc doesn't know this flag
+    "-fno-sanitize-coverage=edge,trace-cmp,indirect-calls,8bit-counters"
----------------
kcc wrote:
> Why is this FIXME? Do you expect to implement these flags in gcc? 
No the intention is to communicate to the reader that these flags aren't understood by gcc. It is FIXME because in the future we might allow building libFuzzer with gcc but disable building this tests (which can't be built with gcc).

================
Comment at: lib/Fuzzer/CMakeLists.txt:13
@@ -5,1 +12,3 @@
+    "-Werror")
+  # FIXME: Test the compiler supports those flags
   add_library(LLVMFuzzerNoMainObjects OBJECT
----------------
kcc wrote:
> Do you really need this FIXME?  (up to you, just asking) 
> 
It is something we should be testing for really. It just isn't a priority for me right now so it's a FIXME

================
Comment at: lib/Fuzzer/test/CMakeLists.txt:34
@@ +33,3 @@
+  foreach (flag ${BUILD_FLAGS_AS_LIST})
+    # FIXME: Use of XX here is to avoid CMP0054
+    if (NOT ("XX${flag}" MATCHES "XX-O[0123s]"))
----------------
kcc wrote:
> Do you need this FIXME> 
Yes. This is communicating to the reader why we are doing "XX${flag}" instead of "${flag}. This is something that ought to be fixed but everyone has to be using a recent version of CMake and have a particular policy set in order to that which we can't do right now.

================
Comment at: lib/Fuzzer/test/CMakeLists.txt:110
@@ -70,1 +109,3 @@
     )
+  target_compile_options(LLVMFuzzer-${Test} PRIVATE ${SANITIZE_COVERAGE_FLAGS})
+  llvm_append_linker_flag(LLVMFuzzer-${Test} ${SANITIZE_COVERAGE_FLAGS})
----------------
kcc wrote:
> This sucks. It really makes the file much bigger. Is that the only way? 
Whether or not "this" sucks is a matter of perspective. I don't think it does because it is adding a considerable amount of clarity. It is adding a few lines but I consider this a **massive** improvement to the way things were being done before.

Also **I need this separation** of compile and link flags because in some new patches that I'd like to upstream I need to pass some other flags to the linker to get things to build under OSX.


http://reviews.llvm.org/D20558