[PATCH] D66887: [test-suite][WIP] Add GCC C Torture Suite as External Test Suite

[Sam Elliott via Phabricator via llvm-commits]

lenary marked 19 inline comments as done.
lenary added inline comments.


================
Comment at: SingleSource/Regression/C/gcc-c-torture/CMakeLists.txt:43-50
+  gcc_torture_dg_options_cflags(${File})
+
+  # Add any flags that were requested
+  list(APPEND CFLAGS ${_TORTURE_CFLAGS})
+  list(APPEND LDFLAGS ${_TORTURE_LDFLAGS})
+
+  # Remove any flags that will make clang error
----------------
kristof.beyls wrote:
> I don't understand cmake much, I'm afraid.
> My guess is that the CLANG_UNKNOWN_CFLAGS could only come from options encoded in specific test files, i.e. those that are being discovered through cmake function gcc_torture_dg_options_cflags.
> If so, wouldn't it be better to only remove those options from the list of options that gcc_torture_dg_options_cflags finds?
> IIUC, gcc_torture_dg_options_cflags currently modifies the global CFLAGS variable. Maybe a better design would be that it returns the list of options it finds without modifying the global CFLAGS variable. And do the filtering of flags that clang doesn't know inside gcc_torture_dg_options_cflags?
> Let me repeat that I don't know much about the cmake language, so there may be idiomatic reasons why such a design would not be best.
I actually had to look up how scope works in CMake when I had a previous bug in this patch. I believe this is doing something reasonable.

In CMake, each subdirectory and each function are a new scope. They inherit the scope of the enclosing subdirectory/function, but new `set` calls (and things like `append`) apply only within the current scope.

So this function is not modifying a global `CFLAGS`, it's taking the per-subdirectory CFLAGS, adding some extra flags, and then setting the CFLAGS variable in the current function's scope.

I have decided to simplify the logic in `gcc_torture_dg_options_cflags` and set a variable in the parent scope with the dg-options cflags only, rather than updating `CFLAGS`. I also do the removal of the erroring/unknown cflags in the above function now.


Repository:
  rT test-suite

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

https://reviews.llvm.org/D66887