[PATCH] D97717: [SYCL] Rework the SYCL driver options

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.


================
Comment at: clang/unittests/Frontend/CompilerInvocationTest.cpp:547
 TEST_F(CommandLineTest, ConditionalParsingIfTrueFlagNotPresent) {
   const char *Args[] = {"-fsycl"};
 
----------------
This test originally checked that enabling SYCL without specifying the version is fine and results in `LangOptions::SYCL_None`.

I'd like to keep the semantics of this test intact (it exercises the underlying tablegen machinery of `ShouldParseIf`).

I suggested only replacing `-fsycl` with `-fsycl-is-host` here and in the assert below.


================
Comment at: clang/unittests/Frontend/CompilerInvocationTest.cpp:556
 
-  ASSERT_THAT(GeneratedArgs, Contains(StrEq("-fsycl")));
+  ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-fsycl"))));
   ASSERT_THAT(GeneratedArgs, Not(Contains(HasSubstr("-sycl-std="))));
----------------



================
Comment at: clang/unittests/Frontend/CompilerInvocationTest.cpp:565
 
-  ASSERT_FALSE(Diags->hasErrorOccurred());
-  ASSERT_TRUE(Invocation.getLangOpts()->SYCL);
-  ASSERT_EQ(Invocation.getLangOpts()->getSYCLVersion(), LangOptions::SYCL_2017);
+  ASSERT_TRUE(Diags->hasErrorOccurred());
+  ASSERT_EQ(Invocation.getLangOpts()->getSYCLVersion(), LangOptions::SYCL_None);
----------------
Here, too, I'd like to keep the test mostly intact, only updating `-fsycl` to `-fsycl-is-host` or `-fsycl-is-device`.

You already test that `-fsycl` passed to CC1 results in an error in `clang/test/Frontend/sycl.cpp`, so I don't think we need an unit test for that.


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

https://reviews.llvm.org/D97717