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

[Jez Ng via Phabricator via llvm-commits]

int3 accepted this revision.
int3 added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lld/MachO/SectionPriorities.cpp:37
 
+PriorityBuilder macho::priorityBuilder;
+
----------------
Roger wrote:
> thevinster wrote:
> > nit: Since we are already namespacing macho, can we drop the `macho::` everywhere in this file? 
> the code does not compile without `macho::`. I'm not sure exactly why but it appears to be necessary.
not for declarations :)

`macho::` here tells the compiler that this is a declaration in the `macho::` namespace, not the global one.


================
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:
> > 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`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122156