[PATCH] D127670: [lld-macho] Use binary search in InputSection::getLocation

[Nico Weber via Phabricator via llvm-commits]

thakis added a comment.

Thanks for the patch!

As a general guideline, I think it helps if every patch is either a behavior-preserving refactoring (these patches can be larger), or something that changes behavior (these patches are ideally smaller). That makes it easier to reason about invariants, and makes the patch easier to review.

In this case, the patch is tiny either way, and it doesn't matter all that much. But it does two separate things: 1. Change the search algorithm implementation (behavior-preserving) 2. Change the diag (not behavior preserving). So abstractly, it seems kind of nice to split them, or at least to talk about both parts separately.

For the refactor patch: There are usually few symbols per section, so a linear scan is likely faster in practice. Also, this function is only called when emitting errors, so it's not performance-critical anyways, probably. So I guess the motivation for that part is clarity?

For the behavior change: Nice catch! Patches that change behavior ideally come with tests – do you think you could include one? In practice, we'd only handle this in files without `.subsections_via_symbols`, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127670