jdoerfert added a comment. Same as the other commit, where is this tested?
================ Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:425 + ErrorOr<std::string> NvlinkPath = sys::findProgramByName( + "nvlink", sys::path::parent_path(LinkerExecutable)); + if (!NvlinkPath) ---------------- Unsure why we look at the linker bin dir first, PATH seems to be the right choice here. ================ Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:439 + + // TODO: Pass in arguments like `-g` and `-v` from the driver. + SmallVector<StringRef, 16> CmdArgs; ---------------- If this is not addressed in a follow up, it seems trivial to add it. ================ Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:468 - - return EXIT_SUCCESS; } ---------------- keep exit success, makes it obvious this worked. ================ Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:497 + Triple TheTriple(TargetFeatures.first); + StringRef Arch(TargetFeatures.second); + ---------------- .str() above to get a key is somewhat ok, but then parsing the key again is weird. Can we make it a densemap and provide a densemapinfo for `struct DeviceFile`. It can just inherit from the DenseMapInfo<std::string>. Might even work w/o explicitly calling the "super" functions with the `.str()` version if we have a `std::string operator`. Might then even work w/o DenseMapInfo by telling the DenseMap to use the std::string specialization of DenseMapInfo in the first place. ================ Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:633 + for (const auto &Image : LinkedImages) { + auto FileOrErr = wrapDeviceImage(Image); + if (!FileOrErr) ---------------- Try to replace auto where the type is short and helpful. E.g., here it would help to know what it is. ================ Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:647 if (std::error_code EC = sys::fs::remove(TempFile)) - reportError(createFileError(TempFile, EC)); - } - - return EXIT_SUCCESS; + return reportError(createFileError(TempFile, EC)); } ---------------- do we really want to return here? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116627/new/ https://reviews.llvm.org/D116627 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits