[clang] Use range-based for to iterate over fields in record layout, NFC (PR #122029)
Reid Kleckner via cfe-commits
cfe-commits at lists.llvm.org
Thu Jan 9 11:20:53 PST 2025
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
More information about the cfe-commits
mailing list