[PATCH] D152206: [Basic] Support 64-bit x86 target for UEFI

[Petr Hosek via Phabricator via cfe-commits]

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