[PATCH] D100523: [lld][MachO] Add support for LC_VERSION_MIN_* load commands

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: lld/MachO/InputFiles.cpp:106-107
+template <class LP>
+llvm::Optional<PlatformInfo> InputFile::getPlatformInfo() const {
+  if (!isa<ObjFile>(this) && !isa<DylibFile>(this))
+    return llvm::None;
----------------
int3 wrote:
> how about having this be a file-static function (like `hasCompatVersion` was)?
> 
> same for `checkCompatibility`.
passing-in `this` as an argument suggests that it should be a method ...


================
Comment at: lld/MachO/Writer.cpp:395
+      break;
+    default:
+      llvm_unreachable("invalid platform");
----------------
int3 wrote:
> I think clang should be able to see that this is exhaustive
ok


================
Comment at: lld/MachO/Writer.cpp:406-407
 class LCBuildVersion : public LoadCommand {
+  const PlatformInfo &platformInfo;
+
 public:
----------------
int3 wrote:
> convention is to put private members at the bottom of the class declaration
ok


================
Comment at: lld/MachO/Writer.cpp:596
+static bool useLCBuildVersion(const PlatformInfo &platformInfo) {
+  static const std::map<PlatformKind, llvm::VersionTuple> minVersion = {
+      {PlatformKind::macOS, llvm::VersionTuple(10, 14)},
----------------
int3 wrote:
> nit: I would use a DenseMap here just to be consistent with the rest of LLVM
ok


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100523