[PATCH] D89420: [lld-macho][easy] Fix segment max protection

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/OutputSegment.cpp:36
+         "TODO: i386 has different maxProt requirements");
+  return initProt(name);
 }
----------------
compnerd wrote:
> int3 wrote:
> > int3 wrote:
> > > compnerd wrote:
> > > > It seems that it would be better to replace `initProt` and `maxProt` with just `prot`?  Or just filling in the differences that require this would be a good idea.
> > > Does it make sense to fill in code that we're not going to be able to test?
> > I wrote it this way so it would be easy to "fill out" the missing bits when we support i386
> I think that it does make sense to fill it out now even if we cannot test it.  Alternatively, we could defer the setup to the point where it is needed.  In the current state, it is very confusing and not very valuable.
I think the assertion makes the intent clear -- they're two separate functions because on i386 their behaviors deviate. Moreover, making it an assertion ensures that when we do add support for i386, it'll be immediately obvious that a test needs to be written to cover this edge case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89420