[PATCH] D123884: [HLSL][clang][Driver] Support target profile command line option.

[Chris Bieneman via Phabricator via llvm-commits]

beanz added inline comments.


================
Comment at: clang/include/clang/Basic/CodeGenOptions.h:194
+  /// The validator version for dxil.
+  std::string DxilValidatorVersion;
+
----------------
Rather than adding this to `CodeGenOptions`, it may make sense to put this in `TargetOptions` instead as it relates specifically to the DXIL target.


================
Comment at: clang/lib/CodeGen/CGHLSLRuntime.h:39
+#endif
\ No newline at end of file

----------------
Make sure to setup your code editor to add newlines to the end of files, this does turn into a compiler warning because ISO C requires it :)


================
Comment at: clang/unittests/Driver/ToolChainTest.cpp:509
+  IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
+  struct SimpleDiagnosticConsumer : public DiagnosticConsumer {
+    void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
----------------
Can you share this struct definition with the one in the other test case?


================
Comment at: clang/unittests/Driver/ToolChainTest.cpp:597
+  Diags.Clear();
+  DiagConsumer->clear();
+}
----------------
Probably worth having some test cases for completely bogus things like `-validator-version blah`, or `-validator-version -some-other-flag`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123884