[PATCH] D141705: [HLSL] [Dirver] add dxv as a Driver Action Job

[Xiang Li via Phabricator via cfe-commits]

python3kgae marked an inline comment as done.
python3kgae added inline comments.


================
Comment at: clang/lib/Driver/Driver.cpp:4226
+    Args.ClaimAllArgs(options::OPT_cl_ignored_Group);
+  }
 
----------------
jhuber6 wrote:
> nit. remember to `clang-format`
arc did find some clang-format issue.
But missed this one :(



================
Comment at: clang/lib/Driver/ToolChains/HLSL.cpp:170
+
+Tool *clang::driver::toolchains::HLSLToolChain::getTool(
+    Action::ActionClass AC) const {
----------------
jhuber6 wrote:
> I feel like this logic should go with the other random `Tool` versions we have in `ToolChain.cpp`. See `ToolChain.cpp:440` and there should be examples of similar tools.
This is following pattern of MachO::getTool for VerifyDebug.
https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Darwin.cpp#L1078

I can add getValidator to HLSLToolChain if that helps.



================
Comment at: clang/lib/Driver/ToolChains/HLSL.h:53
   static std::optional<std::string> parseTargetProfile(StringRef TargetProfile);
+  bool needValidation(llvm::opt::DerivedArgList &Args) const;
+
----------------
jhuber6 wrote:
> Is `needValidation` a good name here? It's asking more like `hasValidator` or `supportsValidation`.
It is combination of hasValidator and requireValidator.
Maybe create hasValidator and requireValidator then move the warning reporting back to Driver?
Something like

```
if (TC.requireValidator()) {
    if (TC.hasValidator()) {
        add the action.
    } else {
        report warning.
    }
}
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141705