Details
- Reviewers
ldionne Mordante var-const jdoerfert - Group Reviewers
Restricted Project - Commits
- rG84fc2c3cd62a: [libc++] Make the naming of private member variables consistent and enforce it…
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
@Mordante You are currently working on chrono, right? Is there anything that interferes with one of your patches?
Thanks for working on this! In general SGTM, but I would like to see the CI pass after rebasing.
Correct, but only on non-member functions. I don't see any conflicts, thanks for asking.
libcxx/.clang-tidy | ||
---|---|---|
46 | Not your change but please verify whether https://reviews.llvm.org/D129503#inline-1244548 is addressed otherwise please rebase and fix that before landing this patch. | |
46 | Maybe not this patch but can we add a regex that when a member starts with __ it should end with _. | |
libcxx/include/__chrono/year.h | ||
105 | This will conflict with D129442, sorry I wasn't aware of this patch. | |
libcxx/include/any | ||
315 | This prompted my regex question, since they weren't updated. |
- Address comments
libcxx/.clang-tidy | ||
---|---|---|
46 | I'll try, but that might require a custom clang-tidy check. I think we should first work through the checks already available before adding custom ones for us. It might work with PublicMemberCase combined with PublicMemberIgnoredRegexp though. I'll try that after the easier ones. | |
libcxx/include/__chrono/year.h | ||
105 | I discovered the tautological compare while working on this patch, so I already expected a merge-conflict. It's small enough that I thought it wouldn't matter that much. | |
libcxx/include/any | ||
315 | With this patch I'm only checking private members, since protected or public members could be required by the standard. I'm working on checking more, but it'll take some time to get most things checked. |
LGTM.
I don't think this should break the LLDB pretty-printers since they use indices to access members (so changing the names shouldn't matter).
Not your change but please verify whether https://reviews.llvm.org/D129503#inline-1244548 is addressed otherwise please rebase and fix that before landing this patch.