bader accepted this revision. bader added a comment. This revision is now accepted and ready to land.
This part looks good to me. Just a couple of minor style comments. ================ Comment at: clang/lib/Driver/ToolChains/SPIRV.cpp:18 + +void SPIRV::constructTranslateCommand(Compilation &C, const Tool &T, + const JobAction &JA, ---------------- If this function is going to be used only by `SPIRV::Translator::ConstructJob`, it's better to make it `static` or manually inline into 4-line `SPIRV::Translator::ConstructJob`. ================ Comment at: clang/lib/Driver/ToolChains/SPIRV.h:31 + Translator(const ToolChain &TC) + : Tool("SPIRV::Translator", "translator", TC) {} + ---------------- I think using just "translator" as a short name might be ambiguous. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112404/new/ https://reviews.llvm.org/D112404 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits