[PATCH] D128462: [HLSL] add -I option for dxc mode.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a subscriber: jansvoboda11.
kadircet added inline comments.


================
Comment at: clang/include/clang/Driver/Options.td:6362
 
-def cl_Group : OptionGroup<"<clang-cl options>">, Flags<[CLOption]>,
+def cl_Group : OptionGroup<"<clang-cl options>">, Flags<[CLDXCOption]>,
   HelpText<"CL.EXE COMPATIBILITY OPTIONS">;
----------------
beanz wrote:
> kadircet wrote:
> > i am failing to understand the semantics of this new option group, and it's causing regressions in clangd. see https://github.com/clangd/clangd/issues/1292.
> > 
> > is this new group suppose to serve flags that are available both in `--driver-mode=cl` and `--driver-mode=dxc`? i.e. they are for the flags in the intersection?
> > are there any flags that are applicable to CL driver mode but not to the DXC mode (and vice versa)?
> > 
> > why didn't we go for just adding DXCOption mode into here, if every option in CL mode is actually applicable to DXC option as well, rather than introducing a new option?
> DXC does not support all the CL options, and vice versa. There are some shared options, which is what this group tries to capture.
okay thanks for the explanation. i'll fix it on the clangd side since the new option group seems to be necessary indeed (or we actually need to disentangle the include/exclude logic inside the driver).

it's still a little bit unfortunate that we actually need a new option flag just because we actually need to perform both inclusions and exclusions of options (i.e. we can't simply let this be `CLOption | DXCOption`, because being in `--driver-mode=dxc` would actually disable all options marked with `CLOption`).
This feels like a really fragile design and also implies such changes needs to take good care of all the call sites that does option parsing to make sure the newly introduced options are handled appropriately. I wonder why we can't just have a parser that requires inclusions. That way rather than introducing new option flags for such cases, we can just union them and users of the option parser would only lack the ability to parse new option groups, rather than having regressions around parsing existing options.

WDYT @jansvoboda11 @MaskRay (as owners for driver and compiler options) ? Is this something that has been on your radar?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128462