[PATCH] D97979: [lld-macho] Check platform and version in constructor ObjFile

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/InputFiles.cpp:351
+
+  if (static_cast<uint32_t>(config->target.Platform) != cmd->platform) {
+    error(prefixFn() + " has platform " +
----------------
ultra nit: given that you're casting `cmd->platform` to a PlatformKind below, why not do it here too for uniformity?


================
Comment at: lld/MachO/InputFiles.cpp:364-368
+  if (major > minVersion.getMajor() ||
+      (major == minVersion.getMajor() &&
+       (minor > minVersion.getMinor() ||
+        (minor == minVersion.getMinor() &&
+         subMinor >= minVersion.getSubminor()))))
----------------
how about constructing another VersionTuple instance and compare that against `platformInfo.minimum`?

that way you can also use VersionTuple::getAsString() below


================
Comment at: lld/MachO/InputFiles.cpp:523
+
+  if (const build_version_command *cmd =
+          findCommand<build_version_command>(hdr, LC_BUILD_VERSION)) {
----------------
no need to repeat the type name on the same line


================
Comment at: lld/MachO/InputFiles.cpp:687
+    if (!hasCompatVersion(
+            [&]() -> std::string { return "dylib " + toString(this); }, cmd,
+            config))
----------------
seems kind of janky to pass in a function here just so we can prepend "dylib". I think it's clear enough that it's a dylib from the name (and if it's not, we should update `toString` so that all our error messages are uniform). Basically I think it would be cleaner to just pass in the `InputFile *`


================
Comment at: lld/MachO/InputFiles.h:179
+template <class CommandType = llvm::MachO::load_command>
+const CommandType *findCommand(const llvm::MachO::mach_header_64 *hdr,
+                               uint32_t type) {
----------------
nice, I like the convenient casting. But I don't think there's a need to move this function into the header


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97979/new/

https://reviews.llvm.org/D97979