phosek added a comment.
This needs a lit test.
================
Comment at: clang/lib/Driver/ToolChains/UEFI.cpp:92-97
+ StringRef Linker =
+ Args.getLastArgValue(options::OPT_fuse_ld_EQ, CLANG_DEFAULT_LINKER);
+ if (Linker.empty() || Linker.equals_insensitive("lld"))
+ Linker = "lld-link";
+
+ linkPath = TC.GetProgramPath(Linker.str().c_str());
----------------
This would ideally be handled by `ToolChain::GetLinkerPath`, can you leave a
comment here along those lines?
================
Comment at: clang/lib/Driver/ToolChains/UEFI.cpp:97
+
+ linkPath = TC.GetProgramPath(Linker.str().c_str());
+
----------------
This could be simplified as suggested, also the name of the variable should be
`LinkerPath`.
================
Comment at: clang/lib/Driver/ToolChains/UEFI.cpp:102
+ Args.MakeArgString(linkPath), CmdArgs, Inputs, Output);
+ if (!Environment.empty())
+ LinkCmd->setEnvironment(Environment);
----------------
The `Environment` variable is never modified so this condition will never hold.
================
Comment at: clang/lib/Driver/ToolChains/UEFI.h:10-11
+#ifndef LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_UEFI_H
+#define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_UEFI_H
+#include "clang/Driver/Tool.h"
+#include "clang/Driver/ToolChain.h"
----------------
Nit: Can you add an empty line here?
================
Comment at: clang/lib/Driver/ToolChains/UEFI.h:54-55
+ bool isPICDefaultForced() const override { return true; }
+};
+} // namespace toolchains
+} // namespace driver
----------------
Nit: Can you leave an empty line here for consistency?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D152206/new/
https://reviews.llvm.org/D152206
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
