bogner added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:710-716
+ "Shader model is required in target '%0' for DXIL generation">;
+def err_drv_dxil_unsupported_shader_model : Error<
+ "Shader model '%0' in target '%1' is invalid for DXIL generation">;
+def err_drv_dxil_missing_shader_stage : Error<
+ "Shader stage is required in target '%0' for DXIL generation">;
+def err_drv_dxil_unsupported_shader_stage : Error<
+ "Shader stage '%0' in target '%1' is invalid for DXIL generation">;
----------------
aaron.ballman wrote:
> Hmmm in theory:
> ```
> def err_drv_dxil_bad_shader_required_in_target : Error<
> "shader %select{model|stage}0 is required in target '%1' for DXIL
> generation">;
>
> def err_drv_dxil_bad_shader_unsupported : Error<
> "shader %select{model|stage}0 '%1' in target '%2' is invalid for DXIL
> generation">;
> ```
To make that readable you kind of have to do something like this:
```
if (T.isDXIL()) {
enum { ShaderModel, ShaderStage };
if (T.getOSName().empty()) {
Diags.Report(diag::err_drv_dxil_bad_shader_required_in_target)
<< ShaderModel << T.str();
} else if (!T.isShaderModelOS() || T.getOSVersion() == VersionTuple(0)) {
Diags.Report(diag::err_drv_dxil_bad_shader_invalid)
ShaderModel << T.getOSName() << T.str();
} else if (T.getEnvironmentName().empty()) {
Diags.Report(diag::err_drv_dxil_bad_shader_required_in_target)
<< ShaderStage << T.str();
} else if (!T.isShaderStageEnvironment()) {
Diags.Report(diag::err_drv_dxil_bad_shader_invalid)
<< ShaderStage << T.getEnvironmentName() << T.str();
}
}
```
It's not really clear to me which way is better. WDYT?
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:4054
+ if (T.getOSName().empty()) {
+ Diags.Report(diag::err_drv_dxil_missing_shader_model) << T.str();
+ } else if (!T.isShaderModelOS() || T.getOSVersion() == VersionTuple(0)) {
----------------
aaron.ballman wrote:
> Idle question: can you pass `T` directly here? (I thought we had diagnostic
> streaming support for `Triple` but I'm not spotting it with a quick look
> through the source).
Tried it, and no. Looks like we don't have that overload.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D159103/new/
https://reviews.llvm.org/D159103
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
