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

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added a comment.

Thanks Sam, I think this is starting to look good!
I just had a few more nit-picky comments.

Were you planning once you commit to quickly add skip lists for other architectures too so that all public bots for all architectures will keep on passing?



================
Comment at: SingleSource/Regression/C/gcc-c-torture/CMakeLists.txt:1
+# This pulls out options in dg-options int CFLAGS. It has to be a macro so it
+# doesn't have a scope of its own.
----------------
s/int/into/ ?


================
Comment at: SingleSource/Regression/C/gcc-c-torture/CMakeLists.txt:26
+
+# The following cause errors if clang recieves them in CFLAGS
+set(CLANG_UNKNOWN_CFLAGS
----------------
s/recieves/receives/ ?


================
Comment at: SingleSource/Regression/C/gcc-c-torture/CMakeLists.txt:39
+  # names to be unique, so we add a disambiguator to the target name. The
+  # disambuguator is based upon the directory structure, swapping `/` for `-`.
+  get_filename_component(Name ${File} NAME_WE)
----------------
spell check disambuguator


================
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
----------------
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.


================
Comment at: SingleSource/Regression/C/gcc-c-torture/execute/LICENCE.txt:1
+The testcases in this directory are covered by the GPL. See the files whose
+names start with COPYING for copying permission.
----------------
I think the filename for this file should be LICENSE.TXT to keep it consistent with other LICENSE.TXT files in the test-suite.


Repository:
  rT test-suite

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

https://reviews.llvm.org/D66887