jhuber6 added inline comments.
================
Comment at: clang/lib/Driver/ToolChains/HLSL.cpp:170
+
+Tool *clang::driver::toolchains::HLSLToolChain::getTool(
+ Action::ActionClass AC) const {
----------------
python3kgae wrote:
> 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.
>
Yeah, I guess it's a style thing. Personally I don't mind having everything in
one place because you need to handle the ActionClass there anyway. But it's not
a huge deal. I'll accept either.
================
Comment at: clang/lib/Driver/ToolChains/HLSL.h:53
static std::optional<std::string> parseTargetProfile(StringRef
TargetProfile);
+ bool needValidation(llvm::opt::DerivedArgList &Args) const;
+
----------------
python3kgae wrote:
> 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.
> }
> }
> ```
>
I think keeping the logic as-is and calling it `requiresValidation` would be
fine.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D141705/new/
https://reviews.llvm.org/D141705
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
