[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:
> 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
> 
the old code wasn't checking it at all - so potentially one could pass InputFile* pointing to an instance of some unexpected type and the old code would crash (the problem stems from the fact that the hierarchy doesn't model the common part of DyLib and ObjFile).
InputFile  exposes a bunch of its internals as public fields => that's why we kind of don't see that pain right now and the approach with an external function works.
If you think it's better to make it an external function - okay, I can change it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100523