[PATCH] D122156: [lld][Macho][NFC] Encapsulate priorities map in a priority class

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/SectionPriorities.cpp:305
   MemoryBufferRef mbref = *buffer;
-  size_t priority = std::numeric_limits<size_t>::max();
+  size_t priority = lowestPriority;
   for (StringRef line : args::getLines(mbref)) {
----------------
Roger wrote:
> int3 wrote:
> > Roger wrote:
> > > int3 wrote:
> > > > I think you're confused about the intention of the code. `numeric_limits::max()` is the *highest* priority, not the lowest. But it's used to initialize the default value of the `lowestPriority` variable because if there are no entries in the order file, then the lowest priority is also the highest priority.
> > > > 
> > > > Now that we have the `PriorityBuilder` class, I think `lowestPriority` can be a member there. And maybe add a comment so future readers won't get confused :)
> > > `lowestPriority` actually represents the highest priority that has not been assigned (ie, the priority lower than the lowest priority that has so far been assigned). `lowestPriority` is used to keep track of the highest priority `CallGraphSort` should use because the code aims to make sure that all priorities assigned by `CallGraphSort` should be lower than those assigned by the order file.
> > > 
> > > At the time this line of code is called, `lowestPriority` will always be set to its initial value of `std::numeric_limits<size_t>::max()`. There is no need to specify `std::numeric_limits<size_t>::max()` both here and at the initialization of `lowestPriority`.
> > > 
> > > I considered making `lowestPriority` a member of PriorityBuilder but it would be a bit more than just moving the field and changing some names because `lowestPriority` is read and written to by `CallGraphSort` as well. I didn't include that change here but it could possibly be done.
> > ah okay, I see how you're thinking about it. I still think `priority = lowestPriority` is confusing though, because it's not obvious that we really mean "lowest priority unassigned by the order file" instead of "lowest possible priority". Also come to think of it, the local `priority` variable is kind of redundant. How about using `lowestPriority` directly but renaming it to something like `lowestUnusedPriority`?
> Actually, it may be better to rename `lowestPriority` to `highestAvailablePriority`.
sgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122156