jansvoboda11 requested changes to this revision.
jansvoboda11 added a comment.
This revision now requires changes to proceed.
I like the simplification of the command line interface. I have concerns about
changing the tests just to make them pass though.
================
Comment at: clang/unittests/Frontend/CompilerInvocationTest.cpp:547
+ // there is no syntax I could find that would allow it). However, the option
+ // is handled properly on a real invocation. See: Clang::ConstructJob().
+ ASSERT_THAT(GeneratedArgs, Contains(HasSubstr("-sycl-std=")));
----------------
The `generateCC1CommandLine` function is used when CC1 gets invoked with
`-round-trip-args`. It is also going to be used in `clang-scan-deps`.
Instead of putting a fixme here, I think it would make more sense to decide how
to handle the option properly. There are a couple of approaches:
* Instead of removing `ShouldParseIf<fsycl.KeyPath>` from `sycl_std_EQ`, it
could be replaced with `ShouldParseIf<!strconcat(fsycl_is_device.KeyPath, "||",
fsycl_is_host.KeyPath)>`.
* Remove `ShouldParseIf<fsycl.KeyPath>` from `sycl_std_EQ`, but issue
warning/error in `CompilerInvocation::fixupInvocation` whenever `SYCLVersion !=
LangOptions::SYCL_None && !(SYCLIsDevice || SYCLIsHost)`.
* Remove the marshalling annotation from `sycl_std_EQ` and handle its
parsing/generation including the device/host checks manually in
`CompilerInvocation`. (I'm personally not fan of moving simple logic like this
from tablegen marshalling annotations to `CompilerInvocation` though.)
See
https://clang.llvm.org/docs/InternalsManual.html#adding-new-command-line-option
for more information.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97717/new/
https://reviews.llvm.org/D97717
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
