[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