jhuber6 added inline comments.
================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:425
+ ErrorOr<std::string> NvlinkPath = sys::findProgramByName(
+ "nvlink", sys::path::parent_path(LinkerExecutable));
+ if (!NvlinkPath)
----------------
jdoerfert wrote:
> Unsure why we look at the linker bin dir first, PATH seems to be the right
> choice here.
Copied it from somewhere, should change it. Right now it's just a weird way to
look through `/bin/`
================
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;
----------------
jdoerfert wrote:
> If this is not addressed in a follow up, it seems trivial to add it.
It is addressed later.
================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:468
-
- return EXIT_SUCCESS;
}
----------------
jdoerfert wrote:
> keep exit success, makes it obvious this worked.
Will do.
================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:497
+ Triple TheTriple(TargetFeatures.first);
+ StringRef Arch(TargetFeatures.second);
+
----------------
jdoerfert wrote:
> .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.
Probably a smarter idea, I needed a way to group all files with the same device
file type. I'll look into it.
================
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));
}
----------------
jdoerfert wrote:
> do we really want to return here?
Might be a good idea to keep trying to remove temp files even if one fails,
will address.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116627/new/
https://reviews.llvm.org/D116627
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
