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

[Jez Ng via Phabricator via llvm-commits]

int3 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;
----------------
alexshap wrote:
> 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 ...
but it doesn't require access to anything private in `InputFile`. Not that there's much internal state to begin with... but whenever possible I like to make things free functions instead of methods to make it clear that they don't need to muck around in the internals of a class.

Also the fact that we check for the subtype of `this` feels wrong -- a method shouldn't have to know about its potential subclasses.

not a big deal though



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100523