[PATCH] D136374: [lld-macho] Don't put entries with less than 2 usages into the common table.

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

> What is the concern? IMHO, it's easier to debug failures in general when there are multiple separate test files with single-purpose testings rather than trying to pack as many related things as possible into one giant file 🤔

It's a tension between that vs being able to easily understand what is being tested -- one of the purposes of a test is to serve as a declarative spec of behavior, which are usually easier to understand than the imperative code -- partly because they are terser.

Other reasons include test suite running time, plus the amount of effort needed to update tests when LLD's behavior changes.

Given the tradeoffs, it's a matter of striking a balance. Adding a whole test file to cover a single if statement seems kind of heavy. If we did that for every other `if`... we would have a lot more tests. It's not always necessarily *wrong* -- if e.g. there's a tricky edge case that only happens for very carefully constructed inputs, then sure, having a separate test file makes sense even if the fix is a single `if`. But this particular test doesn't seem to fall into that category.

(Also, couldn't the current test be simplified? It seems like you could just vary the `.cfi_def_cfa_offset` value to get different encodings. Doesn't seem like `_main` and `_exception1` are necessary...)

Plus I noted that the `compact-unwind.s` test doesn't really test for encodings at all. I'm thinking we could add `CHECK`s for that, plus a single `_unique_encoding` function that ensures its uniqueness via the CFA offset.

Feel free to push back, but these are my reasons :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136374