This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Make the naming of private member variables consistent and enforce it through readability-identifier-naming
ClosedPublic

Authored by philnik on Jul 8 2022, 10:56 AM.

Diff Detail

Event Timeline

philnik created this revision.Jul 8 2022, 10:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2022, 10:56 AM
philnik requested review of this revision.Jul 8 2022, 10:56 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript

@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.

@Mordante You are currently working on chrono, right? Is there anything that interferes with one of your patches?

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
104–108

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.

philnik updated this revision to Diff 444054.Jul 12 2022, 1:05 PM
philnik marked 4 inline comments as done.
  • 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
104–108

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.

philnik updated this revision to Diff 444302.Jul 13 2022, 9:13 AM
  • Try to fix CI
philnik updated this revision to Diff 444486.Jul 13 2022, 6:54 PM
  • Next try
philnik updated this revision to Diff 451770.Aug 11 2022, 1:49 AM
  • Rebased
  • Try to fix CI
philnik updated this revision to Diff 451802.Aug 11 2022, 4:39 AM
  • Fix merge conflict
philnik updated this revision to Diff 457216.Sep 1 2022, 3:14 AM
  • Rebased
  • Try to fix CI
philnik updated this revision to Diff 457590.Sep 2 2022, 7:19 AM
  • Try to fix CI
ldionne accepted this revision.Sep 2 2022, 8:16 AM

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).

This revision is now accepted and ready to land.Sep 2 2022, 8:16 AM
aprantl added a subscriber: aprantl.Sep 9 2022, 3:47 PM

FYI, this broke a whole bunch of dataformatters in LLDB.

See https://reviews.llvm.org/D133618