rnk wrote: Thanks for the review!
> nitpick, I don't think this is an NFC change. It would require some thought > to really convince myself this did not have subtle behavior changes even if > unintended. I agree this change assumes some invariants about immutability of field ordering to be confident that there is no behavior change, but I think it is correct that this change is intended to be a pure refactoring with no externally observable behavior change, which is what [our docs say NFC means](https://llvm.org/docs/Lexicon.html#n). I can use NFCI if that's clearer. Mainly, it's just a justification for not having new tests, because there should be no observable change. > Thanks for doing this! FWIW, I think splitting this to smaller PRs would be > easier. For example: > > 1. `getFieldIndex()` inlining. > 2. Passing fields instead of their indexes and iterate over fields. > 3. Use `std::next()` instead of defining a `Next` variable. > 4. Rename `isNoUniqueAddress` to `isNonVirtualBaseType`. That's not unreasonable, but that feels too fine-grained to me, given the overhead of the PR system, review round trips, etc. The diffstat for this change feels right to me (+52/-59). https://github.com/llvm/llvm-project/pull/122029 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
