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

[Roger Kim via Phabricator via llvm-commits]

Roger marked an inline comment as done.
Roger 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)) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122156